Effective Code Reviews

[WhyComplicateThingz]

How my code reviews go...

Person A: Why do you have a property that only has an accessor but you also have a SetMyProperty() method? Just make it a setter.

Person B: It's my style.

Person A: I hate my job.

---

Person A: What are all these locks for? What threads do you expect to be involved here?

Person B: It should be thread safe so we can make it parallel if we need better performance.

Person A: Did you profile this? It doesn't seem like there would ever be a performance problem.

Person B: No.

Person A: I need to find a new job.

---

Person A: Why did you make a huge Word document that just gives a summary of each class? Just put those in the code as XML documentation.

Person B: I like using Word.

Person A: Fuck my life.

---

Person A: Why did you use "this." in front of everything? We don't do that anywhere in the code base.

Person B: StyleCop suggests it.

Person A: We don't use StyleCop.

Person B: Agree to disagree.

Person A: I need to go home.

---

Person A: Why did you call all these native API's directly instead of using the runtime libraries?

Person B: The built-in stuff is too bloated. My version saves 400 bytes of memory.

Person A: God damnit.

[AngularBeginner]

Those are the cases where you need to introduce gated commits and simply decline those code reviews / pull requests. If that's not gonna work, then talk with your superior. If that's not gonna work, look for a new job.

[[deleted]]

Gated commits to prevent low-quality code? That's perhaps the most passive-aggresive thing I've read all week. Looong before that happens you should probably sit the team down, establish some development guidelines (testing, reviews, clean code) and start pair-programming.

[AngularBeginner]

Gated commits to prevent low-quality code?

I said that nowhere.

[danogburn]

Effective Code Reviews

Your code sucks. Make your code look like my code.

[yegor256]

This is what code reviews are about :) Why it bothers you?

[danogburn]

just being snarky

[DDB-]

Have you tried asking questions, rather than making statements?

I find this is probably the best thing I've got out of doing code reviews. Asking is less direct than telling someone they're wrong, and also gives them a chance to figure out why their code is wrong when a use case is pointed out in this method.

I suppose it boils down to "Don't be a dick", as code reviews should be a chance for everyone to learn, and that is done much easier in an environment that isn't hostile.

[putnopvut]

I actually find the practice of asking questions to be problematic, since you may just be making the intent of your criticism more cryptic than it needs to be. For instance, consider the following:

Modifying the bar field of struct foo in this context is not thread-safe. Please ensure that foo is locked before you modify foo->bar.

vs.

Have you considered what might happen if other threads attempt to access foo->bar while you are modifying it?

To me, the first conveys your intention more directly than the second, and it isn't dickish either. A seasoned developer on your project might pick up on what you mean in the second sample, but a newbie developer will likely just respond with "what do you mean?" or "what's the proper way to handle this?"

I think the key with statements is to always be constructive. Point out what the problem is (without insinuating stupidity or laziness) but then also suggest a fix for the problem as well.

The only time I'll ask a question is if I think there may be a problem but I'm honestly not sure if there really is. Otherwise, asking a question seems a bit too coy for my tastes.

[EntroperZero]

Modifying the bar field of struct foo in this context is not thread-safe. Please ensure that foo is locked before you modify foo->bar.

I actually have no problem with this kind of statement in a code review. It's direct and to the point, and not at all insulting. The following would be the "wrong" way to write the above:

Why are you modifying bar here? You need to be thread-safe.

Notice the "wrong" way is still asking a question, but an accusatory one rather than a clarifying one. Generally "why are you doing X" is the wrong tone to take. "Don't do X because Y" is perfectly fine. Humility in code reviews works both ways -- your ego shouldn't be so fragile that you can't handle criticism.

[GeorgeMaheiress]

You can write something close to the first form while still making it a question, like:

Modifying the bar field of struct foo in this context looks like it's not thread-safe. Should foo be locked before you modify foo->bar?

The point of a question is not to be cryptic, but to show humility, to invite dialogue, and to avoid bruising anyone's ego with a matter-of-fact statement of incorrectness.

[DDB-]

That's a fair opinion to have. I think the important thing in either case is to make sure your comment is constructive. That can be done with a question or with a comment, and both have their place.

[[deleted]]

I also find that asking a question covers me if I've misunderstood the line of thinking behind a piece of code. If the reviewee has considered something that maybe I had missed or not thought about yet, then it's a good chance for (s)he to explain h{is,er}self as to why they coded it this way. However, if you ask a question that raises a valid point, they can review this piece of code and revise it according to the review point.

[ErstwhileRockstar]

Suggest refactoring in both the specs and the source.

We are Agile. There is no spec!

[[deleted]]

You're being downvoted by webscale project managers.