1

I am working as a frontend developer and had a new colleague joining my team. He is more experienced than me but I have more knowledge about the application we are working on.

So we are doing the code-review of each others code and I know that he is very into code-styling and being consistent in the whole application.

Now I reviewed his code and made some suggestions about some code-styling and conventions and I always gave reason why I think this is better.

He answered my suggestions basically saying that he doesn't agree and giving some examples.

I then answered his statement again using these examples and trying to explain again why I prefer it the way I suggested.

He then answered, he would like to postpone this discussion. I guess that he still doesn't agree with me and never will I guess. And I think this discussion will never come up again.

My question is how I can make him agree with me without forcing him? And what should I do when he will never agree on my points? Also I don't like that he decided to postpone this discussion. Should I ask him when we would discuss it then?

|improve this question
  • Many IDEs allows formatting rules. Where I work we avoid your problem entirely by having a pre-defined set of coding styles and the auto-format will format the files to those rules. The only time style then comes up "please auto-format your code" and that's it. The rules were agreed in the company and are open to discussion but everyone must abide by what is agreed. – adamcooney Nov 13 '18 at 15:25
  • We use Linters and IDEs but it is more about figuring out the rules we want to follow. and this causes a lot of discussion. – anonymousSO Nov 13 '18 at 16:01
  • 1
    Hang on, which one of you is arguing for "improvements" that differ from the current house style / way you do things. You or him? – Nathan Cooper Nov 14 '18 at 16:48
4

This happens just about everywhere (so you aren't alone) I will give you the advice I give to everyone:

Stop commenting about stylistic/cosmetic ideals in your code reviews.

Yes, it would be nice if a code base was consistent stylistically, but the reality is that will never happen when 2 or more people work on a project. It may start out that way, but it will never finish that way. Here is a list of what I look for when doing a code review in order of importance:

  1. Logic bugs
  2. Missed requirements
  3. Edge cases
  4. Style (only if the code makes no sense and will need refactoring)

If I go through my entire code review process and do not find anything that does not fit those 4 things, then I might comment on style and make it very clear it is just my preference. I do this because I do not believe a code review should ever contain no suggestions to the author. As long as the code makes sense, do not comment on style unless there is literally nothing else to comment on.

Even then, this is really just to show that you did in fact read the code and are capable of making suggestion. If the co-worker doesn't want to implement the stylistic approach you recommended, leave it alone (unless it will cause readability issues/ is confusing to the point of refactoring).

In my opinion, if your code review is mostly consisting of styling, you may not be reviewing it enough. I can count on one hand the amount of times I was only able to comment on style.

|improve this answer
  • I would already be happy if there is only one person working on the project and there is consistent style throughout the codebase. But unless there are automatic tools for indentation and such, you are usually out of luck. – simbabque Nov 13 '18 at 14:33
  • 1
    I like your assessment of code reviews and what to say in them, but I'd like to add a point. It's allowed to give positive feedback too. If something is really cool, there is nothing wrong with pointing that out! – simbabque Nov 13 '18 at 14:34
  • 1
    @simbabque I agree entire, I do this as well. Especially for newer people. Saying something like "I really like the way you handled X", or "I wouldn't have thought of Y" can go along way towards their confidence and self esteem – SaggingRufus Nov 13 '18 at 14:57
  • What if you comment on an edge case but can't agree about that either (e.g. "that edge case is 'impossible', etc."). You still need a way to resolve differences in opinion. – Brandin Nov 13 '18 at 15:45
  • At that point, you would need to have a discussion. Once you have made your suggestions (in writing), have a discussion. If the author is just bring stubborn and doesn't want to, that's their prerogative. You brought it up during a code review and it is up to that person to ensure the comments from the code review are implemented. If they choose not to, they better hope that edge case doesn't come up because it'll look pretty bad if it was identified and they chose not to do it. – SaggingRufus Nov 13 '18 at 15:50
-1

The important aspect is to remove the 'he said, I said' discussion out of the equation.

You need a few things in place;

  1. A defined coding standard. Ignore anything that's already been published to your repos; define what a well written piece of code would look like going forward.
  2. A defined workflow for moving code through development, peer review, QA review and publishing. That workflow definition would include the next point...
  3. A static code analysis tool, such as Lint (others are available, depending on your development languages). If code does not pass the Lint check, it should not go forward for peer review.

Coding standards are important. There is still room for individuality in the way the code is written, but having a base set of coding standards means that future developers can come up to speed on the code base quickly.

You will have a large technical debt in the existing code base, but that's done and working - move on, not backwards. That technical debt will exist as a 'get your feet wet' project for any future developers who join the team.

|improve this answer

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.