3 ms·

You can ask the senior engineers, but nothing beats rolling up your sleeves, diving in and peeling the onion back. If you have a good manager, you need to tell

You can ask the senior engineers, but nothing beats rolling up your sleeves, diving in and peeling the onion back. If you have a good manager, you need to tell them you need more time to absorb and understand the code base you're working on. They should be more than willing to accommodate, particularly for a more a coder with less experience. My biggest pet peeve when I send code out for review is someone responding with the superficial bullshit stuff, so it's great that you identified it as a problem. You do tend to get reviews like that from junior folks, but I also see it with really insecure engineers. People start using code reviews as a weapon to take pot shots at each other. I also think it becomes a problem when you're doing these smaller code reviews as the blog post is recommending; if you're doing bite sized chunks of code, it's really easy to lose context and overlook things which might be performance bottle necks.

matwood 13y ago
My biggest pet peeve when I send code out for review is someone responding with the superficial bullshit stuff Send something out with a fairly complex algo and you get back people complaining about a brace being 2 spaces instead of 4 or something else that will be automatically cleaned up by your editor before commit.
- jakub_g 13y ago
  You should have an automatic code formatter / checker if possible. Plus code linter. Automate everything if possible and make build fail in case of discrepancies. Then there'll be no discussions like that.
apaitch 13y ago
I dunno, either you will reformat using some kind of automated tool before you check in or you won't. If you will, then why not do it before the review and spare everyone the trouble? In a codebase that conforms to certain standards steps away from the standard stick out and distract from the content. If you won't, then maybe it's good that people point it out so that you can fix it manually? Of course there is a difference between a comment saying: "Nit: Trailing whitespace" and "According to Section V, Subsection VII of the Coding Manual you should never add trailing whitespace. Please see that you don't." or some stuff like that. The latter is a passive-aggressive potshot, the former IMO is just a quick reminder.
groby_b 13y ago
Counter point: We, the team, agreed on a coding style for a reason. If you decide to not stick to the style, you're essentially saying "I don't care"/"I'm sloppy". It's a red flag that there might be other issues. It means you don't care about sweating the small details.
einhverfr 13y ago
> I also think it becomes a problem when you're doing these smaller code reviews as the blog post is recommending; if you're doing bite sized chunks of code, it's really easy to lose context and overlook things which might be performance bottle necks. Absolutely. In fact not just performance bottlenecks but other things too. Worse still is if a commit is not a discrete fix, then you have to review a set of commits, possibly editing and re-editing the same code, together. Additionally I imagine I would have less trouble with a single 100 line commit than I would with 100 one-line fixes. Context tracking has overhead. the single best rule is not to worry about size of the commit but ensuring that the commit is self-contained and tested (generally, not necessarily perfectly) before being committed. Commit things so that they are discrete and reviewable chunks and worry less about size.

matwood 13y ago

My biggest pet peeve when I send code out for review is someone responding with the superficial bullshit stuff Send something out with a fairly complex algo and you get back people complaining about a brace being 2 spaces instead of 4 or something else that will be automatically cleaned up by your editor before commit.

jakub_g 13y ago
You should have an automatic code formatter / checker if possible. Plus code linter. Automate everything if possible and make build fail in case of discrepancies. Then there'll be no discussions like that.

apaitch 13y ago

I dunno, either you will reformat using some kind of automated tool before you check in or you won't. If you will, then why not do it before the review and spare everyone the trouble? In a codebase that conforms to certain standards steps away from the standard stick out and distract from the content. If you won't, then maybe it's good that people point it out so that you can fix it manually? Of course there is a difference between a comment saying: "Nit: Trailing whitespace" and "According to Section V, Subsection VII of the Coding Manual you should never add trailing whitespace. Please see that you don't." or some stuff like that. The latter is a passive-aggressive potshot, the former IMO is just a quick reminder.

groby_b 13y ago

Counter point: We, the team, agreed on a coding style for a reason. If you decide to not stick to the style, you're essentially saying "I don't care"/"I'm sloppy". It's a red flag that there might be other issues. It means you don't care about sweating the small details.

einhverfr 13y ago

> I also think it becomes a problem when you're doing these smaller code reviews as the blog post is recommending; if you're doing bite sized chunks of code, it's really easy to lose context and overlook things which might be performance bottle necks. Absolutely. In fact not just performance bottlenecks but other things too. Worse still is if a commit is not a discrete fix, then you have to review a set of commits, possibly editing and re-editing the same code, together. Additionally I imagine I would have less trouble with a single 100 line commit than I would with 100 one-line fixes. Context tracking has overhead. the single best rule is not to worry about size of the commit but ensuring that the commit is self-contained and tested (generally, not necessarily perfectly) before being committed. Commit things so that they are discrete and reviewable chunks and worry less about size.