This I feel is bad. Code reviews are usually between peers so you shouldn't be afraid to seek out clarification where possible. You shouldn't be making edits to code that goes in production without clearly understanding why.
The other thing that wasn't mentioned, that I think is important, is to not act as a blocker for code reviews unless its absolutely necessary. Lots of engineers take on the attitude that they're going to "gate" code they don't agree with by with holding their +1 and bogging down the review with questions and all sorts of runarounds till its what they want. this is a bad attitude to have, even when you're dealing with Junior engineers.
I'm generally going to +1 something unless I fundamentally disagree with it or think its going to break things in production. What I do, though, is leave lots of comments with questions/suggestions and mention it in the +1 with (see comments).
This builds trust on teams, and stops things getting personal, especially with people who aren't very good at dealing with criticism, even in something as banal as a CR. On a team that works well together, teammates will see those comments, think about them and make thoughtful responses, especially once they understand that you're not trying to get in their way. Giving the +1 gives them the freedom to consider your suggestions without being irritated that their PR is being blocked. They feel like they're in control not you.
In rare exceptions, someone will brush off my questions and merge ... which means that next time, I get to be tougher on the review and specifically ask for responses before the code can be merged, because they've degraded the implicit team trust. Usually repeat offenders are assholes, and assholes generally don't last on healthy teams.
And I never close another persons PR.
It varies by team. If your whole team agrees that the code reviewer is the 'gate' then people understand that they just need to fix the thing and move on. I personally love that style of working because you tend not to write trivial suggestions and everyone knows that what people have written isn't meant as nit-picky and just has to be changed.
If you don't think it's the right change then the other person can just ask why it's important and hopefully get educated. I also tend to just trust my colleagues enough that what they suggest is a good change. I often find I may miss small things through like lack of documentation or whatever (get tired after a while) and those reminders in the CR are the equivalent of a spotter getting you to do one last rep at the gym.
Him: "If the reviewer makes a suggestion, and I don't have a clear answer as to why the suggestion should not be implemented, I'll usually make the change."
You: "[Do] not act as a blocker for code reviews unless it's absolutely necessary."
I think these express essentially the same philosophy, with some slight differences in the particulars.
Most code reviews seem to focus on:
1. Examining what the change does
2. Finding ways to make the change in a nicer way. E.g. Refactoring etc.
This leaves out the key step 0 - what is actually trying to be achieved, does it need to be done and is there a better (maybe completely different) way to do it.
This leads to a focus on relatively trivial matters such as naming conventions and method lengths.
I think that the underlying reason for this is laziness. Talking about clever refactoring is an easier/faster process than understanding the 'why'.
Is it wrong to kick this stuff back and tell devs to make it pretty first?
I usually explicitly do code reviews in 2 passes. I look for low level problems first, and also try to identify ways that they can be tested using tools so that next time they are caught before the code is sent for review. My second pass looks at higher level problems with the overall design and checks the functionality to make sure it matches the objectives.
That's why we discuss these high-level design and architectural decisions before-hand so that they are known to the reviewer at the time of the review. We're a small team so it works well. But I'm not sure how this scales up as the team gets bigger. I would like to know how bigger teams approach this problem!
One thing we did on the current project is pre-commit reviews in pairs. This ensures at least two people in the team knows about the changes, let us talk about the why and how of the changes, and possibly teach a coworker new things in the process.
What it ended up doing is that every programmer now self-reviews their own changes prior to the actual review knowing they'll soon share it all face to face with a coworker. Turns out the talks are now about the design of the code, not how it looks.
My guideline is basically "as big as it needs to be, without having large pieces of duplicated functionality." If an algorithm requires 100 lines, none of which are repeated or very similar to the others, then so be it.
I wonder what the distribution of function lengths is like in the Linux kernel... there will be longer ones and shorter ones, but a 20-line limit seems absurdly short.
Also vaguely related: http://www.satisfice.com/blog/archives/27
Not only that, but as the OP mentioned that it doesn't work if the methods/functions have side-effects, then we're building up opinions on a sloppy and fundamentally broken house of cards.
First off, your methods should not be having side-effects that are not obvious from the purpose of the method. And then secondly, back to my original point about not having to flow through the code. In light of that, a function should do one thing, and that's it. And when it does so, the method name clearly states what it does. After you put those two together, you simply "assume" that method does what you expect it to, and "flow" through the logic at a higher level.
I'm not going to go down into some silly "read_input_data" method while "flowing" through the high-level code, because I know what it is supposed to do. And by extension, at some point I will review it in its singular/atomic entirety, or would have already done so. Either read_input_data returns the data, or it loads it up into a instance-variable.
Sometimes, but in general I don't agree.
If the functions are made private and giving appropriate names than it's really a way of self-documenting code, which helps.
Suppose you have:
// Calculate interest
// 100 lines of code
// Apply tax
// 120 lines of code
// Store results
// 30 lines of code
Also, there's more restriction in that the single-use code block only has access to the variables passed into it, which makes it easier to parse, since less things could be happening. It serves to prune variables from the local scope.
Say I split a function up into a few sub-functions because it's getting big. Now I have the problem that I'm jumping forwards and backwards through the code when I want to explore what that function does:
In this case, SubfunctionC() might end up a few pages down in source code. So now it's a context switch to go there and then go back.
Now this can be somewhat avoided with good function names (so you don't HAVE to jump backwards and forwards) and keyboard shortcuts (to make the process quicker), but it's still a trade-off that I think needs to be kept in mind.
In the best case you should be able to follow the logic without diving into the other functions, only looking at their implementation details as that particular detail becomes relevant.
It all depends at which stage of development you are at. If the piece of code we're talking about is mature and rarely changing then yes it seems like a reasonable thing to do. If however this section is still under development then I would opt for another way of dealing with readability issue.
I think if what you want is to optimise the readability of the code then a simple comment above each section, combined with block scoping is a good set up for splitting.
As the true nature of the code emerges one can decide to turn the block into local lambdas then later into functions.
Again that's because of the first point I've made here in this comment. If you are still developing the functionality, splitting early means that subsequent reviews&development may miss potential interactions + potential refactorings/simplifications that would have been quite clear if the function was still "messy"
Often I find the best way to simplify large functions is to tear out sub-blocks and give them name. Loops or large conditional blocks are usually easy to tear out and can usually get very meaningful names. Long stretch of imperative code however does not separate well and probably should remain a very large function.
As someone who uses Resharper's Extract Method a lot this is a nice counterexample to where it's probably inappropriate
I would love an IDE that showed inline in some sane manner function calls, letting me recourse arbitrarily deep - some thing ala code collapsing and expansion. Extra points if you can show a for chart to side of function calls, class interactions and general code flow, highlighting where your cursor is. More points for a pane that shows what variables have to be in order for a the branch of code you're working in to be reached. A changeable granularity setting would be great for all of these.
I think often times we discuss pros and cons without discussing what they could be if we put some effort into changing the current situation.
If you have to jump around between functions a lot to understand something, the code probably could use some reorganizing.
I just say though it's very seldom that a huge class/method is justified. Usually there are plenty self-enclosed logic or reusable methods that can be broken out either to utility libraries or a separate class.
My rule of thumb is that you should be able to look at a method/class and understand what it does, and that diminishes quickly over a certain size.
Sure some logic might be in other classes but that's another layer of abstraction
However, in this case it is more about something to keep an eye for for the reviewer -- if the function is large and complicated it may be a good idea to take a closer look at it. Presumably the reviewer won't force the function to be broken into smaller pieces unless it actually improves readability and maintainability of the code.
Now, I feel that it should be the other way. Most of the time you want to keep the code that does something together, splitting it up only to factor out common reusable pieces (that are actually reused).
This is also discussed in the linked article.
I agree with your point. It seems silly to be strict about such things. More so if the codebase is written in Java or any other verbose language.
Sometimes it can be helpful to split up a method to be called once if it helps readability, for example if it helps split up a loop or abstracts the specific way we get a value.
But, because it doesn't fit in with the whole "Lean" approach to software (deliver features yesterday), all but the most established enterprises don't seem to care much unfortunately. Once more people experience a breach because of their desire to deliver first and remediate vulnerabilities later then perhaps more awareness will be raised. By then it's too late though.
Has anyone here dealt with similar issues during reviews?
Also, generally managing tone - "Looks good! Don't forget to add the doc comments on functions X Y and Z" vs "Why are X Y and Z missing doc comments?". Both have the same fundamental information (X Y and Z could use doc comments), but:
With the former, you're acknowledging progress, a "good" first draft, and pointing out what remains to be done in an ego friendly way ("I know you're good enough to make this even better") that doesn't demand a response.
Meanwhile, the latter demands a response, all of them negative. Either "it doesn't need them", or "I forgot them (because I suck)", or silently ignoring the question. None of these are ego friendly.
If the code just plain sucks, however... I'm not really sure what to do.
And if the coworker gets defensive even with positively slanted feedback... maybe there needs to be a conversation with them, to try and shift their mindset. Show them you get things pointed out in your own code reviews, that it's a team game where everyone is just looking out for each other. That bad code doesn't mean they're a bad programmer, that critique of their code isn't a critique of their skill, that everyone gets blind to the problems in their own code and benefits from a second set of eyes.
Maybe get them in your shoes - helping critique someone else's code, and help explain that they're not being an asshole when they point out edge cases and ugly code, but that they're being an asshole to future them when they have to go back and add a feature, fix a bug, or handle a corner case they'd missed.
Show them you get things pointed out in your own code reviews, that it's a team game where everyone is just looking out for each other. That bad code doesn't mean they're a bad programmer, that critique of their code isn't a critique of their skill, that everyone gets blind to the problems in their own code and benefits from a second set of eyes.
within a few days we're going to be trying exactly this in hopes to show how code reviews are not about attacking and instead about helping.
I've never seen this get solved overnight. Your management really needs to sit down and explain their expectations around code reviews - that fixing things based on co-worker suggestions is a positive thing, that having negative comments won't affect their performance reviews, etc.
I also find that with people like that you really do need to let more things go. If it's not a critical bug or an actual problem with the code then let it slide. Some people just cannot take criticism (even constructive) and the bad blood you'll get by persisting with it will not help you.
I also tend to find these developers are not necessarily the strongest devs and often have other personality problems. This likely indicates that your company needs to focus a lot more on the hiring and culture.
he describes the Pixar "brain trust" which reviews early progress on movies with the director. They found directors would get defensive if they felt someone more powerful was telling them to make specific changes. Once they set some rules like "the director decides what, if anything gets changed" and "talk about what's not working but don't solve the problem for the director" things improved. Now instead of film veterans telling you your movie (code) stinks it becomes about opportunities to make things awesome.
When I finished the comments on the code, I made sure I added a comment to the top of the pull request (we use BitBucket) indicating that it was clear he understood the point of the code and had written a good first pass at it, but there were a few things that needed to be dealt with for idiomatic code.
e.g. I like the way you did this, but I think this would work better but you're on the right track.
Or so says the management training courses I've recently taken. Supposedly there's studies to back this up, too, though I've never read them myself.
The key is that each person has an assigned responsibility, and that one of those is the "Reader". The reader presents the code for review, and the reader is NOT the author. The author is there, but I think it helps when someone else is presenting the code to reduce the feeling that the review is of the author.
Having at least two very accomplished and (culturally and organisationally senior) people routinely "model" reasonable review behaviour can be stunningly effective.
Also this is an area where frequent team conversations about what good code is outside a review situation helps to build a certain culture. It helps step away from nitpicking and arguing.
The team’s lead should make sure that the individual in question knows that software development is a cooperative process and that code review helps build the team.
"Why are you letting me ship this terrible code I wrote? Do you want us to end up in a death spiral of technical debt and crunch? Don't be an asshole, critique my shit! We all need a second set of eyes - I'm no exception!"
...okay, I'm maybe a little more civil than that. But I'm not above leaving some mistakes unfixed to call them out on.
I've also set the tone early on in my own comments (I run the development division) and I've encouraged the more experienced and talented members of the team to review my own pull requests. When they spot a problem in my code, I always make it a point to praise them (if, of course, the problem is valid). This seems to have worked.
Honestly, I think it's actually really, really hard not to react that way (to varying degrees) especially as most code-reviews basically come at the wrong time: you've already written the code, it works, it has glorious unit-tests -- who cares if it's a for vs while loop or whatever. It's actually even worse if there are serious design or maintainability issues, as that's even more code to re-write/"fix".
Personally, I really like having code-reviews -- it's better to get rid of serious problems if they're there. However, I've found a lot of reviews to be rather worthless "rubber-stamp" exercises -- especially if the dev team has just been told Thou Shalt Do Code Reviews Because Reasons! (I've also found the opposite, for sure: getting nice solid advice or missed cases; but these are invariably with other developers who want reviews no matter what management said).
...but all that said, I think so-called "code reviews" really work best if they're spread out over the coding (unless it's really small chunks at a time) -- a bit of pair-programming (or "hey, can you help me with ...") and design (or whatever you call "early on in the cycle") reviews make it a hell of a lot easier to take a different approach.
I think they used to call this "collaboration"?
Companies that just blast out a "new rule: everything gets code reviews!" memo without helping people understand the benefits, and how to do more "along-the-way" validation of design, refactorings, etcetc are always the very worst at having code reviews that are in any way effective. At such places, I find "code reviews" devolve into either useless "rubber stamp" affairs or something like what you describe. Or worse.
Also, if you have "Junior Developers" blasting out code for a couple days (or more!) with no adult supervision with a hope that some last-second "code review" procedure is going to equal amazing code, you're doing mentoring wrong. It's a lot nicer for everyone involved if there is someone sitting down with (or chatting or watching-the-commits-of or whatever works) the less experienced developers. These mentors should be providing hands-on, practical advice as they go. Some devs might need more hand-holding, some will need "a plan", maybe you need to sketch out classes or method-signatures for them, perhaps they need a bunch of half-day tasks written down, etc...Such mentoring should, IMO, be what your Senior Developers are spending a decent chunk of their not-coding time on.
Anyone have any suggestions for time-estimating code review? That's the biggest issue we've faced trying to implement code review into our workflow.
But note that it basically tries to define good programming practice in general. That's a very big topic with a lot of room for debate and disagreement.
I so agree with this!
Properly named variables is perhaps THE first line of defense against bad code. Too many developers think they are concise and having little code if they only abbreviate their variable names enough.
Honestly, "em", "erg", "fc" and "sc" may make perfect sense to use, but it's a form of obfuscation to future developers (including your future self).
Other pet peeve; adding things to variable names that don't add anything meaningful.
Does your code really care that it's a list? Should the reader be pointed at this each and every time. I much prefer just using:
Clear, to the point, and readable.
A better alternative if you find the code is still confusing is to add context by cutting a function up into inner functions. This is why I really hate working in languages that make it difficult to define little closures. In F#, I'll often end up with a couple of 1 or 2 line inner functions and it makes things much easier to read. This simply isn't practical in e.g. C#.
In exported function names and certain class names or modules, sure, a bit of verbosity might help. But inside a function it just make it visually harder to understand and I've rarely found it to be beneficial. There's going to be enough context to load into my brain inside a function accurate, and unfortunately I still subvocalize when reading code and all those extra syllables add up.
Additionally, removing letters means you need to split lines less based on length and focus more on when it makes sense.
This means that the code reviews should involve little more that a checklist of those approved design decisions against their implementation, perhaps a code style is verified as well.
Coding without a design is just hacking, which I believe is the primary cause of burn-out and should be avoided as much as possible.
So, the question is, do you have a design document? I know it doesn't sound very agile, but traditional engineering procedures, when managed well, have a lot of merit in controlling product quality and cost.
To me, the Code Review Best Practices seem awfully like very general knowledge, just gathered together. Not sure if it's HN-worthy per se.
However, the John Carmack link is quite enlightening.
Just adding "error handling and potential bugs" as a generic bullet on the list just doesn't cut it, these should basically be the only items on the list but specified in much greater detail. A serious code review checklist should contain concrete scenarios of these, preferably tailored for your specific application.
Examples of this are: What happens during system startup, before all other modules are ready to answer to requests? What happens if the user enters malformed data (sql injections etc)? Does this function shut down gracefully (transaction safe)? How will the algorithms scale? Race conditions and deadlocks? What happens if the network to the database goes down? Is localization handled properly? Backwards compatibility?
Having some accountability for writing sloppy code is very sobering.
This and this again. Engineering managers, and all that. But also, don't just review, 'sight-lint', and reason about code. Rather, run the tests! It's the shared accountability and knowledge. Does the baseline capability exist (tests pass)? Can you, the reviewer, spot fallacies that the tests don't capture (if not, criticisms feed back to you later)?
My code has bugs and flaws too! That's why I like code reviews, they give me an opportunity to see my blind spots.
When I coee, somewhere a method needs to initiate this execution flow and will therefore contain "and". Even if all it does is call two other methods where this principle is followed. How do I avoid this? I mean, somewhere in the code it makes sense to execute the methods together.
That said, you have to get useful code reviews to see any benefit. To me that means using automated tools to do most of the style checks (your braces should be on this line, no space after this foreach, etc) and having an active culture of not being human-powered code linters when doing code reviews. There is a lot of work that goes into having a team give effective code reviews.
I agree that code reviews can slow down an individual, but the speed up to the team through shared understanding should make up for that.
I ask myself, "WTF was this guy thinking when he wrote this?" far less often when I'm working on code that's been code reviewed. Most of the time, the reviewer catches code like that, and requests that it be fixed and/or commented, before I end up debugging through it 6 months later.
IMO, open office plans, reading HN and nerf guns hinder development speed far more than code reviews.
I certainly believe it's possible to do code reviews sufficiently "wrong" that they're a net hindrance. Drape them in too much ceremony, put too much emphasis on style (Bob doesn't like your whitespace preferences, and he's the code owner, so your changelist is vetoed!) and too little emphasis on subsistence (these corner cases and possible bugs concern me).
We went with a relatively light touch approach. Nothing was technologically enforced - our lead dev simply told us he wanted everyone to start having everything reviewed by anyone - both for code quality and to spread knowledge - and that he'd be pissed if he found a bug in one of our changelists and found out it hadn't been reviewed, going forward.
Whitespace / naming stuff I tended to submit without review.
Quick and obvious fixes, small changes, etc., I was fairly willing to start a review request, submit, and then fix anything caught in the review in a followup changelist.
For larger changes where I had more reservations, I'd usually hold off on submitting until after a review. Maybe threaten to submit if they were dragging their feet too long :P
So the code is written with higher quality up front? Sounds like reviews are working as intended.
There are enough differences over what can be good quality that teams can be bogged down over discussions that just have no good answer and these take a lot of brain power.
I have observed that naturally, the other teammates will review the code of this specific coworker while not looking at changes submitted by good and reliable teammates.
I see code reviews as a tool to provide feedbacks to someone and help him improve the quality of the code he writes.