At what point does "constructive" criticism of your code become unhelpful?

Question

I recently started as a junior developer. As well as being one of the least experienced people on the team, I'm also a woman, which comes with all sorts of its own challenges working in a male-dominated environment. I've been having problems lately because I feel like I am getting too much unwarranted pedantic criticism on my work. Let me give you an example of what happened recently.

Team lead was too busy to push in some branches I made, so he didn't get to them until the weekend. I checked my mail, not really meaning to do any work, and found that my two branches had been rejected on the basis of variable names, making error messages more descriptive, and moving some values to the config file.

I don't feel that rejecting my branch on this basis is useful. Lots of people were working over the weekend, and I had never said that I would be working. Effectively, some people were probably blocked because I didn't have time to make the changes and resubmit. We are working on a project that is very time-sensitive, and it seems to me that it's not helpful to outright reject code based on things that are transparent to the client. I may be wrong, but it seems like these kinds of things should be handled in patch type commits when I have time.

Now, I can see that in some environments, this would be the norm. However, the criticism doesn't seem equally distributed, which is what leads to my next problem. The basis of most of these problems was due to the fact that I was in a codebase that someone else had written and was trying to be minimally invasive. I was mimicking the variable names used elsewhere in the file. When I stated this, I was bluntly told, "Don't mimic others, just do what's right." This is perhaps the least useful thing I could have been told. If the code that is already checked in is unacceptable, how am I supposed to tell what is right and what is wrong? If the basis of the confusion was coming from the underlying code, I don't think it's my responsibility to spend hours refactoring a whole file that someone else wrote (and works perfectly well), potentially introducing new bugs etc.

I'm feeling really singled out and frustrated in this situation. I've gotten a lot better about following the standards that are expected, and I feel frustrated that, for example, when I refactor a piece of code to ADD error checking that was previously missing, I'm only told that I didn't make the errors verbose enough (and the branch was rejected on this basis). What if I had never added it to begin with? How did it get into the code to begin with if it was so wrong? This is why I feel so singled out: I constantly run into this existing problematic code, that I either mimic or refactor. When I mimic it, it's "wrong", and if I refactor it, I'm chided for not doing enough (and if I go all the way, introducing bugs, etc). Again, if this is such a problem, I don't understand how any code gets into the codebase, and why it becomes my responsibility when it was written by someone else, who apparently didn't have their code reviewed.

Anyway, how do I deal with this? Please remember that I said at the top that I'm a woman, and I'm sure these guys don't usually have to worry about decorum when they're reviewing other guys' code, but honestly that doesn't work for me, and it's causing me to be less productive. I'm worried that if I talk to my manager about it, he'll think I can't handled the environment, etc.

Answer 1

There is a chance that you're being singled out as a woman, but it's also possible that you're just a junior developer and new to the job.

Error-checking and expressive messages are important. If you're going to add something to the code, make sure it's right and up to the team's standards. Similarly, if you're modifying someone else's code, try to improve it where possible -- don't go off rewriting the whole thing, but do try to leave it a little cleaner than you found it.

Is there a written version of the coding standards that your team follows? If not, it might be a good idea to write it all down. You can spearhead the effort by writing down the mistakes you make and forming them into a checklist that you can refer to before submitting your changes for review. As a side effect, you can use that written standard to appeal future rejections if they contradict it.

It sounds like there may be some lack of understanding between you and the team lead. It might be helpful for you to ask for a one-on-one meeting with him and discuss what you can do to improve. You can lead in with something like "I feel like I'm still missing a lot of nuances of what I should be doing. As a junior developer I want to grow and improve. Can you help me get there?" and see what happens.

Answer 2

It sounds like you may be taking this stuff a little too personally. Don't; this kind of thing happens all the time.

The reasons for rejecting your check-in (variable naming, comment quality, configuration location) seem pretty standard to me.

The timing of it was the decision of your team lead, and I wouldn't worry about it if I were you. If somebody is blocked over the weekend, the team lead could choose to allow the check-in and ask you to fix it afterwards. If he felt it appropriate to kick it back even though it might block some other devs, that's his responsibility.

As for the team lead telling you not to mimic others but to do what's right, it sounds like he's trying to give you some initiative to make the code base better. That's a good sign. He trusts you to use your judgement, so go ahead and do what you know is right. That doesn't mean that you have to go changing everybody else's code, but it does mean that you should take responsibility for the quality of code that you write.

Answer 3

An addition to the other answers:

As a Lead Developer, I'm usually more picky with the junior devs because they are much more malleable than people that has been working for a few years. (My ppl skills aren't that good... yet.)

It's very hard to change someone who has been working (and earning decent salary) for a while and is satisfied with their level of code (even though the quality could be improved). Those guys don't care if you try to guide them into becoming better/great programmers. They're happy working in the code-factory.

New devs, like yourself, OTOH, usually yearn for guidance and knowing what's right and not. Also, they are able to absorb feedback and change their ways to the better. They are not set in their ways.

If you take these advice to heart and make them a part of your everyday life, you'll find that in no time you will be writing code that's superior to much of the existing code base.

So ...

.. it could be that you're getting more feedback just because you have the potential to make something of it. :)

Answer 4

It's entirely possible that you're being singled out because... you're a junior developer.

From your description, it sounds like you did not follow the standard as the team lead perceives it.

The solution is simple:

• If that's the standard, follow it.
• If you don't understand the standard, ask for clarification.
• If your interpretation of the standard or instructions differs from the team lead's, ask for clarification

Don't make a battle out of it; if you try to make the team lead "wrong" then even if you win, you lose. Learn the appropriate lesson and continue to grow.

Answer 5

Author's Note

A few years later; I've edited this to more accurately reflect how I feel about the situation. I'm putting more nuance into my answer because I'm learning more about nuance in these situations. It's easy to claim a 'black or white' answer, but we all know it isn't that simple. My answer now reflects that.

From what you've described; the behavior you're experiencing doesn't appear to have anything to do with your gender. That isn't to say you aren't experiencing any gender related treatment (I hope you aren't), only that what you describe doesn't seem to be gender related.

When I was a team lead, I treated everyone equally. There's no room in tech for treating someone badly because of their gender. I don't know how to deal with it if it's happening to you.

It's important that you trust that your Team lead treats men and women equally. If there's evidence he isn't, then the old saying applies: Change your environment or change your environment.

By equally I mean that he treats everyone equally without deference to gender. If he is doing his job correctly, you shouldn't see him critique anyone else; and they shouldn't see him critiquing you. In front of others, it's very important for the team leader to show confidence, even if he just spent the last five minutes before correcting behavior in private.

Now on to the issues you've raised:

You checked in code that didn't meet the standard he set out, so he rejected your branch. If I were in his shoes, I wouldn't have done the same thing in the same way, but I would make sure that my subordinates (odd word; because I don't think of a leader as 'superior' to the people they lead; but it accurately (not adequately) describes the situation) know what the right thing to do is. If they don't know what the standards are, that's my fault as a leader. It's up to me to correct it. In this case, you may have made a mistake, but the sheer fact that it happened means that you were either 1) not told what the right thing to do was or 2) was not appropriately mentored. Neither is your fault.

One of the most important parts of being a programmer is realizing that the codebase you work on has to be maintainable by many different people. Any variable messups or other things that detract from being able to read the code are not transparent to the customer, because it takes longer to fix problems in code that's harder to read.

If your team has written coding guidelines, follow them. If they do not, then there should be some sort of community convention for your language (For .NET and C#, Microsoft has a standard that a lot of companies follow).

Ask your Team Lead where the coding guidelines are so you can make sure that you follow them. Take two checkins to your meetings where two other developers didn't consistently follow the guidelines, so that when he says there aren't any, you can point out that others seem to be having problems with it too, and everyone would benefit from having those guidelines.

If he's treating you fairly, then he'll see this and this should be on the top of his list of things to do. If he isn't treating you fairly, then you have ammunition if it keeps happening.

Saying "I'll get to it later" is bad. Later never happens. Take the time to do it right. There is no later in programming.

It's hard when you're a junior developer. You feel pressured to perform and there are a lot of people looking at you, and every mistake you make is tied to your name in source control forever.

Answer 6

Nowhere in your post do you mention how others around are treated. You keep repeating that you "feel" you're being "singled out" because you're "a woman".

I think you're being treated like a junior programmer regardless of your gender and you should be thankful for that, because that's what equality means. I also feel you're making a huge fuss over minor, 5-minute-long code aesthetic changes that you're being asked to do now instead of putting them on a to-do list and never getting around to them.

Nowhere in your post did you mention you were ordered to do these things on the weekend. It might be perfectly fine to check in the fixes on Monday morning.

Your team lead might be a little pedantic for my tastes, but from your post I don't see anything wrong with his or her requests.

Please stop playing the gender card for nothing. I feel this is undignified and undermines the concept of gender equality.

Answer 7

I don't feel that rejecting my branch on this basis is useful. Lots of people were working over the weekend, and I had never said that I would be working. Effectively, some people were probably blocked because I didn't have time to make the changes and resubmit. We are working on a project that is very time-sensitive, and it seems to me that it's not helpful to outright reject code based on things that are transparent to the client. I may be wrong, but it seems like these kinds of things should be handled in patch type commits when I have time.

It is hard to say anything useful as someone who neither saw your code nor knows something about your projects schedule. But, if your lead behaves responsibly and does a good job, he knows, that others weren't really blocked and the sprint won't be late. So, do not worry about that. Perhaps you overrate the impact on your commit. Otherwise: if your project is time critical and passes all tests, it would be too picky to reject code, which could be fixed after the release in a blink of an eye.

Make It Work Make It Right Make It Fast

So, if your lead made the decision to reject your commit, as a professional he should know, what he was doing.

Now, I can see that in some environments, this would be the norm. However, the criticism doesn't seem equally distributed, which is what leads to my next problem. The basis of most of these problems was due to the fact that I was in a codebase that someone else had written and was trying to be minimally invasive. I was mimicking the variable names used elsewhere in the file. When I stated this, I was bluntly told, "Don't mimic others, just do what's right."

As a Junior it is hard to find ones way into a companies codebase.

Best: there are documented coding standards - and you will hopefully learn to get grips on them.

Usually: there are undocumented coding standards - and you have to learn via trial and error or should I say commit and _reject? This is oftentimes painful (as in your case). This is sometimes dangerous in terms of code quality and could lead to cargo cult programming, where one not only mimics variable naming, but copies and pastes structures and patterns from the code base in good faith, it has to be done like that. Do not do that! Do not even mimic existing variable naming.

Stick to Clean Code. It is good practice and gives you an easy to defend position. If it is readable, testable and maintainable code, you mostly have won any discussion.

And this leads to another (the last) advice: Follow the boy scout rule!

Always leave the codebase in a better state, than it was. Even if the surrounding code you are working with is nasty, make yours clean - and if you have time fix the surroundings.

Answer 8

If your lead developer reviews your code on the weekend, then normally you would read that review on Monday morning, apply the required changes, submit them for review again, and when everything is fine, the changes are submitted.

If things are very urgent, the lead developer has two choices: Maybe make the required changes himself (it’s not really good style, but I can do that kind of thing where I work), or merge your work as it is, and add a highest priority task to your to-do-list to make the requested changes. He did neither, so he accepts the delay. What shouldn’t happen is to accept changes for no other reason than time pressure.

When you complain about having to change old code: Someone has to do it at some point, or it never changes. That’s one side, so it’s not something you should complain about. The other side is that if your lead developer wanted those changes then he should have told you beforehand. So he has no good reason to complain about this in a code review.

A good reaction would be to send an email “I’ll fix this first thing Monday morning. However, some people are blocked by this change so it would be better to do the renaming in a separate change”. Truth is, as a developer I would always strongly recommend not to mix that kind of refactoring with any other changes.

Answer 9

Take a little time to learn the various nuances of your coworkers' personalities. In my experience you can avoid irrational, unnecessary, inconsistent or just plain worthless criticism if you work around your coworkers quirks.

For example some coworkers may be hungover on Mondays. They may be very irritable and overly eager to reject certain code branches or commits. If you have to work with someone like this try to avoid committing code on Mondays.

On the other hand a hungover coworker may be too blase to care about the verbosity of error messages... so Monday morning may be the perfect time to commit your code :-p

The personality quirks in the office or workplace are literally endless. Hopefully you can learn who to avoid and when to avoid them. Also don't be too hard on yourself :-) You can always quit and find another job!

Answer 10

This is one of the reasons why I think code reviews should be done in person, rather than 'remotely' in a tool like Github, where the reviewer can just add lots of comments and not have to stare the developer in the eye. A lot of these issues could be solved by the reviewer and the dev having a discussion, in person, over the code. This also allows the problems to be fixed there on the spot, perhaps by the reviewer him/herself.

A sample conversation might go like this:

reviewer: "Your variable names don't adhere to our code standards"

dev: "I was following the style in the code itself as I wanted it to be consistent within the file"

reviewer: "When in doubt, you should follow the standard. Even if the previous developer didn't"

dev: "I wasn't sure about what names would be appropriate. Could you make some suggestions?"

reviewer: "Yes, of course. Let's make the changes now.."

I can easily imagine developers being jerks. That said, their criticisms actually sound quite fair to me. Nor do I see it as being penalised having to do things correctly, even if other people get away with not doing it correctly, because, ultimately, they will suffer by becoming sloppy developers whilst you will become a much better one.

Answer 11

I've never worked in an environment where code is rejected on the basis that certain conventions aren't followed. If I were in your shoes, I would be tempted to look for employment in a place where I'm more empowered to make the right decisions and where the customer is the primary focus, not the code.

I'm not saying clean code and standards aren't important, but the customer and the product timeline shouldn't suffer because of a spelling error in something that no non-technical person, customer, or executive is ever going to see.

With that said, it sounds like you might be working in an environment where expectations aren't clear, or you don't fully understand the requirements, for whatever reason.

Regardless of the situation, it's up to you to take control and ask clarifying questions. Be proactive, if you're not already. Your team members and team lead will likely respect you more for asking questions to clarify the rules for checking in. You could also ask for an "after-action review" where you and your team lead discuss what you should have done instead and how you can tell the difference as far as when to take certain actions and when not to.

I'd suggest giving it time, since you are new, to see if you can get past any hurdles and resolve these issues with experience, communication, and learning the standards. However, if after several months the situation hasn't changed and the environment is still plagued by ambiguity, it may be time to seek employment in another company.

Not every organization is this draconian, and you may find other working environments more suited to your personality, style, and communication requirements.