7 ms·

How to be a faster code reviewer

62 points by jaimefjorge 13y ago

subb 13y ago
The link to codacy at the bottom doesn't work.
- jaimefjorge 13y ago
  Thanks for heads up. Updated!
quaunaut 13y ago
As a less experienced coder reviewing coders with half-decades or more experience on me, I always had a difficult time knowing whether I was looking at something that I just didn't understand the purpose of, or if I should question their wisdom. For awhile, this resulted in me effectively checking to be sure it looked 'grammatically correct'- as in, followed style guides, was linted right, etc. The bullshit stuff. Then a few got through where I had reviewed them, wrote down "What's this for?" as a thing to review a month or so later, and they turned out to be huge problems- sometimes performance, sometimes functionality that would break under the right circumstances. Have others gone through this, and if so, how would you recommend 'teaching' a newer coder how to do quality code reviews of the elder coders around him?
- derefr 13y ago
  As in any tradecraft: have the apprentice review it. Then, have the master review it. Check the apprentice's work against the master's. Don't punish the apprentice for getting their review "wrong"; just show them the master's annotations and let them learn from them. See where they thought the same things, and where they thought differently. Let them argue with the master if they disagree on their judgement, and let this be an opportunity for learning also (on both the apprentice's and master's parts!)
  - dllthomas 13y ago
    I think you mean "trade or craft"; "tradecraft" means "spying" (in my experience).
    - derefr 13y ago
      "Tradecraft" is a general word meaning "the craft-knowledge involved in a trade"; it just isn't very well-used out of a few specific contexts. "Craftwork" is another such word, being "the craft produced as the work of a trade, or the process of applying one's craftsmanship to produce work." Both, I think, are useful in a programming context. As programmers, we do craftwork, and we need to know our tradecraft to do it well--where tradecraft is separate from plain fact-knowledge (it's more a set of intuitions picked up with experience), but is also separate from raw craftsmanship (which is composed of things like willingness-to-say-no-when-told-to-ship-something-unmaintainable, but which doesn't cover the trade-specific knowledge required to say what "unmaintainable" looks like.) "Uncle" Bob Martin's writings about code craftsmanship (http://blog.8thlight.com/uncle-bob/archive.html http://blog.8thlight.com/uncle-bob/archive.html) are exactly that--teaching coders about what craftsmanship, in general, is. He doesn't really talk about tradecraft, just about encouraging coders--who already have implicitly learned tradecraft--that they should be considering themselves craftsmen, and applying their tradecraft. On the other hand, for all the twee style of The Codeless Code (http://thecodelesscode.com/ http://thecodelesscode.com/), it's basically a set of parables that do teach tradecraft.
      - dllthomas 13y ago
        I disagree that "tradecraft" is still so general. I agree "craftsmanship" has no such connotations.
        mbrameld 13y ago
        I spent 12 years in the Air Force, the last 4 in an intel unit writing software to analyze radar signal intercepts. I've never heard the word tradecraft used to mean anything intel-related, although Google tells me that it is used that way. It may be more common in some intel communities than others, but I'd venture to guess that if you did a survey the large majority of folks in the general population will have no idea about the intel-related usage of the word.
- dragonwriter 13y ago
  > Have others gone through this, and if so, how would you recommend 'teaching' a newer coder how to do quality code reviews of the elder coders around him? One approach might be this rule: "If you don't understand, get an explanation from whoever it came from until you do understand." If there's a more-senior reviewer available, you might have them come along for the discussion to ask additional questions as necessary, but the main focus should be getting the original developer to explain their reasoning to the junior reviewer.
  - dllthomas 13y ago
    Very much so. Either it's a bug, it's not written clearly, or you have something to learn. Any which way it's a problem, and asking about it is the way to fix it.
- sukuriant 13y ago
  It never hurts to ask "Hey, what's the code here supposed to do, it looks complex/off?" :)
- Patrick_Devine 13y ago
  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.
- sunir 13y ago
  As a junior, you're expected to ask stupid questions. Despite what you may fear, otherwise you can't learn fast enough and it's too expensive to keep you around. Also, if it's not obvious on a cold reading, it has a good chance of being poorly designed in some way as you've discovered.
- gargarplex 13y ago
  The answer is REALLY simple here. Say "I don't understand what this is doing" and then have add a comment. Just like there's a rule "you're never the smartest person in a room, there's probably someone smarter than you" there's a rule "you're never the stupidest person in the room, there's probably someone stupider than you". Someone in the future will be confused too. Just push for a comment.
- groby_b 13y ago
  Speaking as an "elder" (Yikes!) - do ask questions. Worst case, you'll learn something. And most people who have been coding for a while are happy to explain things, because we know that it a) might help expose problems we thought we had covered, and b) makes sure that there's one more person who knows how to do things, and why stuff is the way it is. If people get upset that you "dare questioning them", they're usually not as competent as they'd like to be seen. Treat them as volatile, and if you have questions where you're unsure, ask instead an experienced person you do trust. But never, ever, rubber-stamp code you don't understand.
  - quaunaut 13y ago
    I certainly came across people(so far, universally my bosses- lead developers who I didn't directly report to so far haven't been like this at all) who certainly had the "don't dare question me" attitude. It made it a lot more difficult to go to them when I was genuinely stuck too, because in general you always feel like you're getting in their way.
- keypusher 13y ago
  Instead of writing something down "What's this for?", go ask the person "What's this for?". I have recently become a primary reviewer for our operating system work (device drivers for SAN storage, VLAN networking, init scripts, custom rpms, etc) which involves a whole lot of bash magic and linux system stuff I do not understand. What I do is go ask the guy to explain to me the basic concepts and/or specifics of the change if I don't understand it. That way it's at least possible that an error will be caught, and when I have to go make changes in that area I will have some idea of what I am up against. Code review is a great time to learn more about what you don't understand, and for more eyes to see dark parts of the code. It's also a good chance for the guy to wrote it to go "hey, that's weird..." when explaining it to me, and realize he missed something important.
  - jakub_g 13y ago
    This. TL;DR: it's really important to be comfortable with saying "I don't know" and asking questions. Unfortunately not every developer is comfortable with it, often it's cultural thing at play (talking about developers from countries like India).
  - einhverfr 13y ago
    Also keep in mind that there is value in being a rubber duck here. There's also a good chance that in explaining it to you, he will find an error during a kind of review that he wouldn't on his own code otherwise.
- anomie 13y ago
  I used to worry about this too, but finally concluded that me not understanding it was in and of itself grounds for rejecting that review. 90% of the time well structured code (and tests, and good comments, etc etc) should be enough on their own to make it easy for me to understand. In the rare cases when the nature of the change means that's not enough then people ideally provide any additional explanation in a note accompanying the review (or in a crucible comment). (In practice I reckon most of the time these explanatory notes probably belong in code comments anyway.) We have a very strong culture of constructive code review though - I guess you might have some work to do to get those more experience coders to get into the mindset that would accept this sort of feedback in a positive way
- nostrademons 13y ago
  You should always be able to ask "What's this for?", and a senior engineer should be able explain what it's for. I'm a relatively senior engineer in my department, and I frequently have more junior engineers doing my code reviews. A lot of the time when they point out "What's this for?" or "Why didn't you do it this way?", it really is an error or an oversight, and I should've done it that way. Senior != omniscient. And in cases where I do have a good reason for what I did, it's a teaching opportunity, where I can make the code reviewer aware of a complexity that hasn't come up.
- einhverfr 13y ago
  Two things I have found helpful: 1. Review collaboratively. If you have questions, ask other reviewers and the developers. By asking questions you aren't questioning their wisdom, just pointing out that you don't understand what they wrote, so be respectful. 2. Introduce new programmers to the process gradually and require more collaboration than you would for more senior programmers. This helps ensure that new people are brought up to speed with appropriate mentoring from their senior staff. Review is a form of collaboration. It is a way to shake out logic before stuff gets into formal testing. Discussion is good.
taude 13y ago
Be sure to add tasks to the sprint for the code review to happen. If it's not an actual sprint item, I find it likely won't get done effectively. If you have a couple hours, it's a great time to train the developers on some of the libraries, code standards, practices, too.
- einhverfr 13y ago
  > Be sure to add tasks to the sprint for the code review to happen. Just make sure it is a sprint and not a sprintf.
sunir 13y ago
A related question. I'm wondering what other people do to ensure quality of outsourced code. I'm intrigued at Codacy. A lint in the cloud could solve some of my problems, but probably not all. I'm in the position of occasionally outsourcing non-critical development but not having time myself (I used to be technical; now focused on marketing) or able to distract the dev team to look at code in depth. Do you outsource development? How often do you code review that code? Never, once in a while, on delivery, frequently? Do you do anything else to ensure outsourced code quality?
- bojan 13y ago
  My company outsources to Vietnam. At the beginning, we had time to review all the code, and we always had feedback on it. Only after several iterations would it be ok. We would review the code manually, and later also used tools such as Jenkins and Sonar. However, as the number of FTEs over there increased and we switched to paying per month instead of per project, the amount of the code we would received became overwhelming (about 5-6 FTEs over there per 1 FTE here), and the focus moved to getting functionality right in time. Later we would discover horrible things in that code when we'd have to fix some issue. Which was difficult in itself because we did not know the code at all. Very low code quality and constant issues and delays lead us to start hiring people here and slowly phase out Vietnamese programmers. We (as the local dev team) already feel much more in control of our product.
- jaimefjorge 13y ago
  Hi Sunir. Great questions. Hope to get more people answering this. Please also take a look at a parallel discussion in proggit: http://www.reddit.com/r/programming/comments/1zqbx1/10_ways_to_be_a_faster_code_reviewer/cfvymys http://www.reddit.com/r/programming/comments/1zqbx1/10_ways_... From my experience in Codacy, some of our users use it to grade and quickly send out comments from the tool to the outsourced team. They like being able to review the code as it's being done (since it's per commit) with the help of our linting code patterns. We also give out grades which is an overall vision of the project without having to go into much technical detail. "Why is this a C?" is a great question to ask :) I'd love to hear more about your experience and your pains and check if we can be of assistance. Here's my email: jaime at codacy dot com.
Kiro 13y ago
I don't understand the argument of keeping your commits small. In my team we do code review after a task is done, not for every commit so the size of the commits won't affect the review.
- jakub_g 13y ago
  A commit should be like UNIX tools: should do one thing and do it well. If the commit is adding hell lots of new features, fixing bugs, and doing heavy refactoring all at once, it's way too much. When developing say a new widget (or heavily extending an existing one), you can develop basic functionality, commit, add bells and whistles, commit, and some more stuff, commit, send for code review, do the fixes, commit, ask for re-review maybe, and then squash the commits into one commit if needed. With DVCSes like Git it's extremely easy, and reviewing smaller commits is easier than having to go through a behemoth (of course it always depends on your particular situation). I sometimes tend to open half-baked pull requests to ask for quick early feedback if I'm doing things the right way, and what to be careful about, and continue developing in background while the more senior person takes a look at the code.
  - pdovy 13y ago
    Does that really work well in practice, though? I find that I might get the base functionality down to a workable state, but then as you add the bells and whistles you're likely to find and fix your own bugs, realize you need to refactor, etc. You can make all those changes in distinct, clean commits, but if the reviewer works commit by commit, they have to follow the evolution of your code rather than simply review the final state. You might argue that provides a basis for a better review, but it's also much more time consuming for the reviewer.
    - jakub_g 13y ago
      When you find an issue with previous commit while adding new things, you can make a temporary commit, fix the issue, commit once again and reorder commits, then meld the fix into the previous one. I'm pretty comfortable with doing things like that but some guys not that much. You have to like this workflow and work from the start like this. You can of course split a huge commit into smaller ones using sth like `git add -p` but it's way more work than just doing lots of small commits regularly and then squashing them into medium-sized commits before the review. I like to do temporary commits regularly as "checkpoints", meaning, "this code works", and anytime I make a stupid mistake and break something, I can analyze a short diff to find the bug I just introduced rather than trying to figure out the bug just by looking at the code. It's usually a lot faster for me.
deathanatos 13y ago
The first point isn't something you can do: that's something you others have to do for you. Unless you count convincing people to change their workflow, which I've found to be near equivalent to moving mountains. In my experience, it seems to boil down to not being effective with their VCS (or in some cases, the VCS not being an effective tool). But it's hard to convince people to go through the trouble of learning something: the short-term cost is just too much, and they seem to be blind to the long term gains. It's like convincing someone to use vim (who isn't an emacs or equivalently-powerful-editor user).
voidlogic 13y ago
They missed the most important one, practice. It might sound like I am being snarky or stating the obvious, but like any new task at first you will be slow at code review and it will take up "too much time". Its a skill, in some situations, its not really too much code to review once you have gotten good at reading code. Everyone can do with being better at reading code too- traditionally people have been better at writing their own new code realtive to their ability to reading and grok their teammates code- and that doesn't scale well.

62 points by jaimefjorge 13y ago

subb 13y ago

The link to codacy at the bottom doesn't work.

jaimefjorge 13y ago
Thanks for heads up. Updated!

quaunaut 13y ago

As a less experienced coder reviewing coders with half-decades or more experience on me, I always had a difficult time knowing whether I was looking at something that I just didn't understand the purpose of, or if I should question their wisdom. For awhile, this resulted in me effectively checking to be sure it looked 'grammatically correct'- as in, followed style guides, was linted right, etc. The bullshit stuff. Then a few got through where I had reviewed them, wrote down "What's this for?" as a thing to review a month or so later, and they turned out to be huge problems- sometimes performance, sometimes functionality that would break under the right circumstances. Have others gone through this, and if so, how would you recommend 'teaching' a newer coder how to do quality code reviews of the elder coders around him?

derefr 13y ago
As in any tradecraft: have the apprentice review it. Then, have the master review it. Check the apprentice's work against the master's. Don't punish the apprentice for getting their review "wrong"; just show them the master's annotations and let them learn from them. See where they thought the same things, and where they thought differently. Let them argue with the master if they disagree on their judgement, and let this be an opportunity for learning also (on both the apprentice's and master's parts!)
- dllthomas 13y ago
  I think you mean "trade or craft"; "tradecraft" means "spying" (in my experience).
  - derefr 13y ago
    "Tradecraft" is a general word meaning "the craft-knowledge involved in a trade"; it just isn't very well-used out of a few specific contexts. "Craftwork" is another such word, being "the craft produced as the work of a trade, or the process of applying one's craftsmanship to produce work." Both, I think, are useful in a programming context. As programmers, we do craftwork, and we need to know our tradecraft to do it well--where tradecraft is separate from plain fact-knowledge (it's more a set of intuitions picked up with experience), but is also separate from raw craftsmanship (which is composed of things like willingness-to-say-no-when-told-to-ship-something-unmaintainable, but which doesn't cover the trade-specific knowledge required to say what "unmaintainable" looks like.) "Uncle" Bob Martin's writings about code craftsmanship (http://blog.8thlight.com/uncle-bob/archive.html http://blog.8thlight.com/uncle-bob/archive.html) are exactly that--teaching coders about what craftsmanship, in general, is. He doesn't really talk about tradecraft, just about encouraging coders--who already have implicitly learned tradecraft--that they should be considering themselves craftsmen, and applying their tradecraft. On the other hand, for all the twee style of The Codeless Code (http://thecodelesscode.com/ http://thecodelesscode.com/), it's basically a set of parables that do teach tradecraft.
    - dllthomas 13y ago
      I disagree that "tradecraft" is still so general. I agree "craftsmanship" has no such connotations.
      - mbrameld 13y ago
        I spent 12 years in the Air Force, the last 4 in an intel unit writing software to analyze radar signal intercepts. I've never heard the word tradecraft used to mean anything intel-related, although Google tells me that it is used that way. It may be more common in some intel communities than others, but I'd venture to guess that if you did a survey the large majority of folks in the general population will have no idea about the intel-related usage of the word.
dragonwriter 13y ago
> Have others gone through this, and if so, how would you recommend 'teaching' a newer coder how to do quality code reviews of the elder coders around him? One approach might be this rule: "If you don't understand, get an explanation from whoever it came from until you do understand." If there's a more-senior reviewer available, you might have them come along for the discussion to ask additional questions as necessary, but the main focus should be getting the original developer to explain their reasoning to the junior reviewer.
- dllthomas 13y ago
  Very much so. Either it's a bug, it's not written clearly, or you have something to learn. Any which way it's a problem, and asking about it is the way to fix it.
sukuriant 13y ago
It never hurts to ask "Hey, what's the code here supposed to do, it looks complex/off?" :)
Patrick_Devine 13y ago
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.
sunir 13y ago
As a junior, you're expected to ask stupid questions. Despite what you may fear, otherwise you can't learn fast enough and it's too expensive to keep you around. Also, if it's not obvious on a cold reading, it has a good chance of being poorly designed in some way as you've discovered.
gargarplex 13y ago
The answer is REALLY simple here. Say "I don't understand what this is doing" and then have add a comment. Just like there's a rule "you're never the smartest person in a room, there's probably someone smarter than you" there's a rule "you're never the stupidest person in the room, there's probably someone stupider than you". Someone in the future will be confused too. Just push for a comment.
groby_b 13y ago
Speaking as an "elder" (Yikes!) - do ask questions. Worst case, you'll learn something. And most people who have been coding for a while are happy to explain things, because we know that it a) might help expose problems we thought we had covered, and b) makes sure that there's one more person who knows how to do things, and why stuff is the way it is. If people get upset that you "dare questioning them", they're usually not as competent as they'd like to be seen. Treat them as volatile, and if you have questions where you're unsure, ask instead an experienced person you do trust. But never, ever, rubber-stamp code you don't understand.
- quaunaut 13y ago
  I certainly came across people(so far, universally my bosses- lead developers who I didn't directly report to so far haven't been like this at all) who certainly had the "don't dare question me" attitude. It made it a lot more difficult to go to them when I was genuinely stuck too, because in general you always feel like you're getting in their way.
keypusher 13y ago
Instead of writing something down "What's this for?", go ask the person "What's this for?". I have recently become a primary reviewer for our operating system work (device drivers for SAN storage, VLAN networking, init scripts, custom rpms, etc) which involves a whole lot of bash magic and linux system stuff I do not understand. What I do is go ask the guy to explain to me the basic concepts and/or specifics of the change if I don't understand it. That way it's at least possible that an error will be caught, and when I have to go make changes in that area I will have some idea of what I am up against. Code review is a great time to learn more about what you don't understand, and for more eyes to see dark parts of the code. It's also a good chance for the guy to wrote it to go "hey, that's weird..." when explaining it to me, and realize he missed something important.
- jakub_g 13y ago
  This. TL;DR: it's really important to be comfortable with saying "I don't know" and asking questions. Unfortunately not every developer is comfortable with it, often it's cultural thing at play (talking about developers from countries like India).
- einhverfr 13y ago
  Also keep in mind that there is value in being a rubber duck here. There's also a good chance that in explaining it to you, he will find an error during a kind of review that he wouldn't on his own code otherwise.
anomie 13y ago
I used to worry about this too, but finally concluded that me not understanding it was in and of itself grounds for rejecting that review. 90% of the time well structured code (and tests, and good comments, etc etc) should be enough on their own to make it easy for me to understand. In the rare cases when the nature of the change means that's not enough then people ideally provide any additional explanation in a note accompanying the review (or in a crucible comment). (In practice I reckon most of the time these explanatory notes probably belong in code comments anyway.) We have a very strong culture of constructive code review though - I guess you might have some work to do to get those more experience coders to get into the mindset that would accept this sort of feedback in a positive way
nostrademons 13y ago
You should always be able to ask "What's this for?", and a senior engineer should be able explain what it's for. I'm a relatively senior engineer in my department, and I frequently have more junior engineers doing my code reviews. A lot of the time when they point out "What's this for?" or "Why didn't you do it this way?", it really is an error or an oversight, and I should've done it that way. Senior != omniscient. And in cases where I do have a good reason for what I did, it's a teaching opportunity, where I can make the code reviewer aware of a complexity that hasn't come up.
einhverfr 13y ago
Two things I have found helpful: 1. Review collaboratively. If you have questions, ask other reviewers and the developers. By asking questions you aren't questioning their wisdom, just pointing out that you don't understand what they wrote, so be respectful. 2. Introduce new programmers to the process gradually and require more collaboration than you would for more senior programmers. This helps ensure that new people are brought up to speed with appropriate mentoring from their senior staff. Review is a form of collaboration. It is a way to shake out logic before stuff gets into formal testing. Discussion is good.

taude 13y ago

Be sure to add tasks to the sprint for the code review to happen. If it's not an actual sprint item, I find it likely won't get done effectively. If you have a couple hours, it's a great time to train the developers on some of the libraries, code standards, practices, too.

einhverfr 13y ago
> Be sure to add tasks to the sprint for the code review to happen. Just make sure it is a sprint and not a sprintf.

sunir 13y ago

A related question. I'm wondering what other people do to ensure quality of outsourced code. I'm intrigued at Codacy. A lint in the cloud could solve some of my problems, but probably not all. I'm in the position of occasionally outsourcing non-critical development but not having time myself (I used to be technical; now focused on marketing) or able to distract the dev team to look at code in depth. Do you outsource development? How often do you code review that code? Never, once in a while, on delivery, frequently? Do you do anything else to ensure outsourced code quality?

bojan 13y ago
My company outsources to Vietnam. At the beginning, we had time to review all the code, and we always had feedback on it. Only after several iterations would it be ok. We would review the code manually, and later also used tools such as Jenkins and Sonar. However, as the number of FTEs over there increased and we switched to paying per month instead of per project, the amount of the code we would received became overwhelming (about 5-6 FTEs over there per 1 FTE here), and the focus moved to getting functionality right in time. Later we would discover horrible things in that code when we'd have to fix some issue. Which was difficult in itself because we did not know the code at all. Very low code quality and constant issues and delays lead us to start hiring people here and slowly phase out Vietnamese programmers. We (as the local dev team) already feel much more in control of our product.
jaimefjorge 13y ago
Hi Sunir. Great questions. Hope to get more people answering this. Please also take a look at a parallel discussion in proggit: http://www.reddit.com/r/programming/comments/1zqbx1/10_ways_to_be_a_faster_code_reviewer/cfvymys http://www.reddit.com/r/programming/comments/1zqbx1/10_ways_... From my experience in Codacy, some of our users use it to grade and quickly send out comments from the tool to the outsourced team. They like being able to review the code as it's being done (since it's per commit) with the help of our linting code patterns. We also give out grades which is an overall vision of the project without having to go into much technical detail. "Why is this a C?" is a great question to ask :) I'd love to hear more about your experience and your pains and check if we can be of assistance. Here's my email: jaime at codacy dot com.

Kiro 13y ago

I don't understand the argument of keeping your commits small. In my team we do code review after a task is done, not for every commit so the size of the commits won't affect the review.

jakub_g 13y ago
A commit should be like UNIX tools: should do one thing and do it well. If the commit is adding hell lots of new features, fixing bugs, and doing heavy refactoring all at once, it's way too much. When developing say a new widget (or heavily extending an existing one), you can develop basic functionality, commit, add bells and whistles, commit, and some more stuff, commit, send for code review, do the fixes, commit, ask for re-review maybe, and then squash the commits into one commit if needed. With DVCSes like Git it's extremely easy, and reviewing smaller commits is easier than having to go through a behemoth (of course it always depends on your particular situation). I sometimes tend to open half-baked pull requests to ask for quick early feedback if I'm doing things the right way, and what to be careful about, and continue developing in background while the more senior person takes a look at the code.
- pdovy 13y ago
  Does that really work well in practice, though? I find that I might get the base functionality down to a workable state, but then as you add the bells and whistles you're likely to find and fix your own bugs, realize you need to refactor, etc. You can make all those changes in distinct, clean commits, but if the reviewer works commit by commit, they have to follow the evolution of your code rather than simply review the final state. You might argue that provides a basis for a better review, but it's also much more time consuming for the reviewer.
  - jakub_g 13y ago
    When you find an issue with previous commit while adding new things, you can make a temporary commit, fix the issue, commit once again and reorder commits, then meld the fix into the previous one. I'm pretty comfortable with doing things like that but some guys not that much. You have to like this workflow and work from the start like this. You can of course split a huge commit into smaller ones using sth like `git add -p` but it's way more work than just doing lots of small commits regularly and then squashing them into medium-sized commits before the review. I like to do temporary commits regularly as "checkpoints", meaning, "this code works", and anytime I make a stupid mistake and break something, I can analyze a short diff to find the bug I just introduced rather than trying to figure out the bug just by looking at the code. It's usually a lot faster for me.

deathanatos 13y ago

The first point isn't something you can do: that's something you others have to do for you. Unless you count convincing people to change their workflow, which I've found to be near equivalent to moving mountains. In my experience, it seems to boil down to not being effective with their VCS (or in some cases, the VCS not being an effective tool). But it's hard to convince people to go through the trouble of learning something: the short-term cost is just too much, and they seem to be blind to the long term gains. It's like convincing someone to use vim (who isn't an emacs or equivalently-powerful-editor user).

voidlogic 13y ago

They missed the most important one, practice. It might sound like I am being snarky or stating the obvious, but like any new task at first you will be slow at code review and it will take up "too much time". Its a skill, in some situations, its not really too much code to review once you have gotten good at reading code. Everyone can do with being better at reading code too- traditionally people have been better at writing their own new code realtive to their ability to reading and grok their teammates code- and that doesn't scale well.