8

I am in the following situation. Small team within a large unit with many teams. There is an official code review process, but to a lot of developers it is more of a formality so it is a rare thing to see serious discussions or review.

Within my team I encountered a developer who was not following the routine and was committing straight to the develop branch. I opened the topic of code reviews, then we started doing pull requests. My pull requests were automaticaly approved. It was though obvious that he was not reading them, just pressing the button.

At the same time it appears he is taking it a bit personal when I am doing the reviews. I usually do quite thorough code reviews to be honest. I take it seriously. I have been doing code reviews to other developers in the company I work for it is just this one case where the developer appears to be extremely defensive. The problem is that we work very close.

Should I stop doing code reviews? How to approach this situation in the best way?

Just one fresh example to get the point. The developer writes a class that looks like something in between Factory and Container, then names it SomethingBuilder where the Something part is not related to the finally built object. I of course comment on this and let's say he takes it personally. What am I supposed to do here? It appears the Tech Lead of the team does not care. He defends him a lot actually. He says everyone has their own style. I am not sure what kind of style it is to name Factory a Builder, but nevermind. Is avoidance the best strategy here, especially when the Tech Lead shows compassion to the guy?

  • what do you mean "a bit personal"? what do you mean "quite thorough" and "quite seriously", can you give an example of what you're rejecting or commenting on? what does "work very close" mean? physically? do you have a team lead in the team you can raise this to? – bharal Oct 28 '18 at 20:19
  • When I say throughrough, I mean that I am realy spending time to find the problems in the code and point out its weak points. I usualy avoid using the word "you" but reference the code itself. – Pesho Oct 28 '18 at 20:22
  • I mean I don`t do the reviews just formaly. – Pesho Oct 28 '18 at 20:22
  • 1
    @bharal It is a very confusing class actualy. When you read it you expect a Builder that builds particular object. Then you realize it does not have the classic Builder api. Then you realize it is not actualy building what it says, and then you realize it does hell of a lot more than what it actualy Builds. It took me 30 min to understand the logic and I consider myself very fast code reader. – Pesho Oct 28 '18 at 20:29
  • 2
    One could conclude that you're mostly looking for validation here, as opposed to objective feedback. To dismiss that notion, maybe quote yourself in your question. Post the review verbatim. – isherwood Oct 29 '18 at 20:10
12

I think that if your Tech lead definitely does not have your back, then you are going to have to modify your behavior. This is the unfortunate reality of life in a company. The best you can do is to ask your Tech lead to give you guidelines for what your code reviews should be like, and stay within those guidelines. Then, if the developer is getting his back up over something that you said in a review, then you can go to your Tech lead and ask him to back you up based on his guidelines.

5

I've been on both sides of this equation and while it's hard to say exactly what's happening with out knowing the people involved or how they see things, it does sound like you need to be little bit more tactful here. If I had to guess (and it is just a guess) your coworker is probably getting defensive because he feels attacked. To you it's an impersonal part of the job, to them you're some jerk who's getting mad at them for not doing it the way you think it should be done.

However I could be off base here. The first thing you should do is talk to them about it. This conversation does not need to be super formal but it should be private. Let them know that you're trying to help them improve and that your criticisms are not personal. Also don't just focus on the negatives. If they have any response to this then listen to them, and try to understand their point of view. However even if nothing comes of this conversation there are things you can do.

Does you company have any formal code standards? Are these standards documented? If so then your life just got a lot easier. Any time you see something that violates one of these standards politely point out the violation. If he gets angry you can even be like "hey I agree its a stupid standard but I don't make the rules". You can even make it look like you're watching his back by being like "hey dude, I don't want you to get in trouble, you should probably changes this so it matches the standard".

The other big thing you can do is explain why something should be changed. Don't just be like "hey change X to Y". Explain to him that X causes errors in realistic scenario Z or that Y is cleaner and makes the code easier to maintain. Again try to phrase this in such a way that your coworker feels like you're actively trying to help them improve instead of just reprimanding them because they did things their way instead of your way.

Point out the good things in their code as well as the bad. If they did something in an intelligent way then tell them you liked the way they did the thing. This will help prevent them from feeling like you're out to get them.

3

It seems to me here that you are not the high man on the totem pole, as it were. As such, there's really only a limited number of things you can do, and it seems like you've done them:

1) If the team isn't doing PR/CR process, you should push to institute it (you did).

2) If people are rubber-stamping PRs, you should urge them to stop (you did).

3) You should be the example and when someone sends you a bad PR you should comment all over it and explain what's wrong (you did).

4) If people are not working up to snuff, you should bring this to the attention of the tech lead (you did).

At this point, it's the tech lead's responsibility to handle the situation as they want. In this case, the culture comes from the top; the most experienced developer needs to either explain to the rest of the team why the PR process is important, or impose that they have to do it properly even if they don't understand why (optimally the former, but the latter works too). However, if the tech lead doesn't understand why it's a valuable process or doesn't want to put in the effort to make sure it's done properly, there's not much you can do.

As for what you should do going forward, much as it pains me to say so, you should bring up issues, but if the people on the receiving end of the issue think it's a non-issue, then you should rubber-stamp them. You're in danger of going from "the responsible one" to "the anal-retentive one", and you don't want the reputation of being hard to work with, it's not worth it. I would also recommend searching for a new position; you don't want to work with bad developers who are going to bring you down with them. Let them stay on their sinking ship, and you should take your responsible approach to development to a company where such a skill is valued.

2

I would say stick to guns here. Your code review sounds reasonable. If your tech lead is allowing poor naming conventions it will cause confusion among developers that did not write this class. Nameing conventions are useful for many things and standard naming convention are the way to go.

2

The developer writes a class that looks like something in between Factory and Container, then names it SomethingBuilder where the Something part is not related to the finally built object. I of course comment on this and let's say he takes it personally.

A lot depends on how you commented on this. Compare:

This class looks like something in between Factory and Container, and it's named SomethingBuilder which isn't really related to the finally built object.

to:

I wonder if it might be worth the effort of splitting this in to a Factory and Container like so:

[.. small amount of pseudo-code to demonstrate what you mean ..]

I think this will give us [concrete advantage X].

What do you think?

Also, it seems to be that Something doesn't really covey the intent, as it's more of a Otherthing. Maybe rename it to Otherthing?

The first example, which is paraphrased from your question, amounts to little more than complaining about code and saying "your code sucks". I don't think it's particularly strange to take comments like that personally.

The second example offers an alternative, and also phrases it as a suggestion and asks for feedback, rather than just telling your coworker what to do.

He says everyone has their own style. I am not sure what kind of style it is to name Factory a Builder, but nevermind.

This just sounds dismissive of your coworker's views and conventions, and like you're not even willing to take it serious. In real-world programs things are often not all that neatly organized; there may be a good reason to write it like he did, but if your phrasing isn't communicating that you're open to alternatives then you're not going to have that conversation.

Maybe your coworker is just writing silly code; I don't know. But telling him "you're writing silly code" is not a very effective was of changing that. Ask why he did it the way he did. Listen. Provide meaningful alternatives and explain why you think it's better; give him space to contemplate it and experiment with it.

Don't expect him to change just by slinging the dictionary definitions of "Factory" or "Builder" at him; explain how it will make things better in objective terms. "More elegant" is often used, but not a very good argument. If you have difficulty coming up with better arguments than "more elegant" then maybe ... it doesn't really matter all that much?

I usually do quite thorough code reviews to be honest.

Some code reviews can be "too thorough", commenting on things that don't matter a whole lot.

In my own code reviews I always stop and think before leaving a comment: "will this really objectively improve the code quality, or is it just my personal preference?" Not infrequently, it's just my personal preference; so I let it go.

I've seen people comment on some very minor subjective things. It's okay to comment on a minor spelling error/typo in a comment, as that's objectively wrong, but I think it's important to acknowledge that programming is a very subjective craft, and that objectivity is hard to come by (this is why programmers are constantly fighting over everything).

  • 1
    More elegant" is often used, but not a very good argument. If you have difficulty coming up with better arguments than "more elegant" then maybe ... it doesn't really matter all that much? .... I'd give you several + just for that if I could :) – Allan S. Hansen Oct 30 '18 at 9:12
0

Your coworker seem to disaprove the entire review process. Your tech lead is skeptical. This is both of these issues you should adress, beginning with the one that have the most to say about the process.

The tech lead

The tech lead may be skeptical by your way to do reviews or with reviews at all. You need to find out why is he defending your coworker. Be open to criticism. Maybe you are too formal. Maybe the way you give feedback doesn't seem gentle enough.

Then you should sell him that reviews are however necessary to ensure code quality, and team level. Suggest that the team may grow in the future and these issues need to be adressed when it's less painful.

His firm approval is key to the success of reviews. If he doesn't agree, you can simply let go with the review process, but that doesn't mean you don't have an issue with the coworker.

The coworker

Developers in general (me definitely included) tend to put a lot of ego into their work. The problem with reviews is not new, some people do take it personnaly as criticism of their work. This is critical, because developers are expensive and good morale is important.

This is why, no matter if your tech lead approves reviews, you should stress on the fact that despite reviews were hard, you do think he is doing good work, and he shouldn't feel insecure. Check if he isn't coding in a rush as well, remind him if necessary your preference for quality over quantity.

-1

I'm not sure there is much to do here. If the most egregious example you have is the naming of a class - and it's not even that badly named, I'm honestly uncertain of the difference between "builder" and "factory" - then I think you might want to reconsider what you're reviewing in a code review.

Look, a review doesn't always have to have a problem. If the developer is matching team naming conventions, whatever they are, then you don't need to comment on the names.

If you catch, say, a memory leak, or some weirdly ineffective code then yes, note that. But don't fall into the trap of "when asked to comment on something i have to comment on it in some way". You don't. You're in industry, 75% is a commendable effort, hell, 35% is a pass most times.

As long as the things work and doesn't malfunction and it's done in time, well, what more do you want? You're not being paid by lines of beautiful code, you're not being paid by lines of code at all - you're being paid by practical output.

It sounds like you're adding busy work to the team, be careful. Look at it this way - if you were running a company, would you want beautiful code that didn't do anything, or rubbish code that barely scaled - but you could sell?

If you think there's anyone running a company alive that wants the first one, you might be right - but they're not running that company for very long.

A Discussion on Naming

Naming conventions are stupid. Words change their meaning over time, and trying to cement the name to the function is pointless when the function is right there and can be read.

Anyone can read the code to understand what it does. But everyone will have a different idea of what words mean - so insisting the words match the meaning for your understanding of the words is a broken premise, unless you're the only person who will ever read the code, which you are not.

Worse, you want the names to match the inner workings when, really, names should do one of (or better, both of) two things:

  1. describe the output
  2. describe the business logic contained within

Describing the inner workings is mostly pointless, because, again, the code can be read. Capturing the business logic is much, much more important. To whit:

saveDataInAthreadSafeAndDataConsistentManner(){...}

isn't useful. But

saveAddressDetailsAsTheresAConcurrentProblemWhenSavedWithEverythingElse()

well, that's a useful method name.

But at the end of the day, working code >> well named code that doesn't work, and quickly produced working code >> slowly produced well named working code.

Another Thought

Don't ever look like you're the person in the team enforcing standards that slow the team down - that's not a good look. If your team lead agreed with you then you'd be ok to pursue this, but the TL doesn't, so now it looks like you're just fussing over naming conventions. I wouldn't do this.

  • What are you saying now ? That you don`t know what is the difference between Builder pattern and Factory pattern ? Or that in your opinion it is not a big deal to name a Factory pattern to be Builder pattern? Actualy it is not even a Factory I gave it as the closest thing to it but that is a different thing. In the same context is it ok if I name the Toilet to be pissuare and the pisuare to be toilet ? It would be interesting if someone is blind what will he do. – Pesho Oct 28 '18 at 20:34
  • @Pesho I probably don't. To me they both make something, the underlying difference is just how they make that thing. But I wouldn't care how the thing is made, if I were a consumer of it I'd just happily use it. Enforcing the naming conventions as you are means that you want the name to clearly match the how. Sigh, i'll expand my answer... – bharal Oct 28 '18 at 20:37
  • I want the name to match at least "What". It is not building what it is saying that it is building. – Pesho Oct 28 '18 at 20:38
  • I agree with what you have written in the "Discussion on Naming". And I repeat again the class is not doing what it says it is doing. If we puit aside the Builder part, it is saying that it is building one object when in reality it is building another. – Pesho Oct 28 '18 at 20:48
  • @Pesho that's a different issue - i think you should drop the "builder v factory" issue and ask your TL again about the "something" bit. But if your TL is against it, then let it slide. It's not going to win you points, or a promotion. – bharal Oct 28 '18 at 20:50
-4

Sad to say but feel you need to leave that team, maybe leave that company. You are not a good cultural fit. Both your team leader and whoever promoted your team leader have chosen to reward a particular pattern of behavior. The chances of both those people leaving and the whole culture changing are very small.

Instead of rewarding you for caring, your team leader has been disrespectful and dismissive of your attempts to improve the quality of workmanship within your team. This puts you in a politically weak position and easy to blame for slowing things down, when in fact the code is ”written once and read many times”, easy to read and understand and maintain means a quicker path to bug free product. From your own description your colleague's code style slowed you down by 30 minutes to work out exactly what just one class did. Surely such a description should have been part of the class' name.

In addition your management chain has ignored basic math and basic psychology.

For the math, if one module has a 99% chance of no bug (I e not using sufficient unit tests), then you can calculate (1-p)^n that 100 modules together have a chance of 37% of bug free behavior. The probabilities go against you very quickly. Improving the ”quality” to 99.5% improves the system to just over 60%.

For the psychology, who wants to work for or with people who don't care?

  • This is jumping to conclusions. When I joined my current company it was a "commit to master" kind of mentality in many teams. It took some effort and trail and error to find the right way to do code reviews (people had to get used to it), but we got there, and I'm still working there over 2 years later. In particular, it's not at all clear to me if people are protesting code reviews as such, or that the problems lie in the tone/style of the OP doing the reviews. I suspect that at least part of the problem may lie in the latter. – Martin Tournoij Oct 30 '18 at 1:04
  • i think that advising someone to leave a company because a their code review style clashes slightly is a little extreme. – bharal Oct 30 '18 at 16:26
  • This is from my own experience, put heart/soul to get things manageable. Of course it is engineering not math, there is no value to be perfect. We got rid of using globals as communication between threads, use message passing instead and state machine at the top of each thread. Took 2 months, reduced the support load by 75%. No race hazards. But team leader wanted reputation as someone who ”always delivered”. Globals reappeared, few unit tests, anyone could check in any change they wanted. Company bankrupt 3 years later with reputation for code with odd behavior, impossible to reproduce/debug. – user3588820 Oct 31 '18 at 3:28

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.