Code Reviews



  • I've lately become the unofficial development lead at my company. Our company has been around for a while but only got into software development early last year, and when I came here there was no QC whatsoever on code. So I've stepped up and tried to implement some processes and tools to help us get more organized (SVN, BugTracker, an internal wiki, code standards, etc). I want to get code reviews in place so we can can catch each others' sloppy code (sometimes there's quite a bit) before it goes out to production and also help us all become better coders.

    So what advice can you guys give on code reviews? How to lead them, how often to hav ethem, how to do them, what to look for, etc. Keep in mind I'm not even a year out of college yet and I'm already having to manage other employees at least a little, and I have no experience in management.

    There are four people on our dev team by the way. Me, another programmer, an intern, and a graphic designer.



  • My advice, completely unrelated to your question, is to get a new job title and raise. If you're taking on more responsibilities.



  • @blakeyrat said:

    My advice, completely unrelated to your question, is to get a new job title and raise. If you're taking on more responsibilities.
     

    That.

     Also, you're definitely way too small for formal code reviews. Instead, create an environment where people work on other people's code and aren't too shy to come down hard on another's work. The designer obviously doesn't program so he's exempt. The intern must have all his work reviewed by you or the other programmer.

    YMMV. Tailor to suit your needs and situation. It may very well be that the intern is the most competent person there.

    With such a tiny, tiny team, you're basically all team leaders and each of you carries a lot more responsibility than, say, a company of 50 with several teams of 5 codemonkeys, under a competent dev captain, in turn under an IT manager.



  • @blakeyrat said:

    My advice, completely unrelated to your question, is to get a new job title and raise. If you're taking on more responsibilities.

     

    The raise already happened. As far as I can tell, job titles around here are whatever you make up for yourself.

    @dhromed said:

    Also, you're definitely way too small for formal code reviews. Instead, create an environment where people work on other people's code and aren't too shy to come down hard on another's work. The designer obviously doesn't program so he's exempt. The intern must have all his work reviewed by you or the other programmer.

    YMMV. Tailor to suit your needs and situation. It may very well be that the intern is the most competent person there.

    With such a tiny, tiny team, you're basically all team leaders and each of you carries a lot more responsibility than, say, a company of 50 with several teams of 5 codemonkeys, under a competent dev captain, in turn under an IT manager.

    Yeah, I definitely understand that we're too small for formal stuff. On the other hand, a lot of lazy work (sometimes including my own) gets published right now and I'd like to see less of that. These past few weeks I've been trying to figure out what I can do to help out without locking our tiny team into some draconian processes that don't make sense for our size. Right now I think we need some way of reviewing each other's code so we can hold each other accountable to the coding standards we've come up with and improve each other's work, at least until everyone gets used to them. It's just painful when I have to fix a bug in someone else's code and I find one window is all over the variables and controls of another window or two instead of using some common data class that all those windows will access, or when I find the same 10-line code snippet copy-pasted in eight different places in the same class file.

    So, another question: How concerned should I be about that stuff? We have a working relatively non-WTF product, am I just being anal about how the code looks then?

     



  • @mott555 said:

    [ As far as I can tell, job titles around here are whatever you make up for yourself.



    Can I come work for you?  My Title would be Spanish Inquisitor of Code Review....
    You would, of course, have to provide the Comfy Chair.

     

     


  • ♿ (Parody)

    @mott555 said:

    How concerned should I be about that stuff? We have a working relatively non-WTF product, am I just being anal about how the code looks then?

    In my experience, that stuff only gets worse. It's best to fix it as soon as possible, or at least not repeat it. In the future, you often won't have the time or resources to fix this sort of thing. We all do stupid stuff like this sometimes, but some people are just too stupid to do anything else, or learn from mistakes. Hopefully, you don't have any of those on your team.

    Of course, sometimes TRWTF will be you. You can usually tell when you get push back from a non-WTF dev who has reasonable arguments for why they did something a certain way. There's no hard and fast rule.

    In a few months/years, when you have to go back in there to do something, you'll be glad for all the WTFs you cleaned up or prevented.



  • @mott555 said:

    It's just painful when I have to fix a bug in someone else's code and I find one window is all over the variables and controls of another window or two instead of using some common data class that all those windows will access, or when I find the same 10-line code snippet copy-pasted in eight different places in the same class file.
     

    Tell the culprit. Hit him with a large stick. 


  • 🚽 Regular

    For teams as small as yours, I'd think you'd benefit more from some pair-programming. Especially when you're working on a particularly difficult and complex piece of your software, it's always good to have a second pair of eyes.

    This is good no matter how experienced or advanced the programmers are. I've pair-programmed with the CTO, who is competent and well-versed in software design, and have caught a few silly mistakes from him, which could have cost him a bit of troubleshooting in the future when he started testing it.

    Doing an hour or two of pair-programming every several days gives the benefit of introducing more than one programmer to the piece of software, improves communication between programmers, and can give each programmer some insight in the method of thought of the other. It's sort of like a code-review, only it's done as the code is being written, which IMO is more productive than having programmers get up from their keyboards and spend an hour or two NOT programming and only reviewing what they had done in previous days.



  • @blakeyrat said:

    My advice, completely unrelated to your question, is to get a new job title and raise. If you're taking on more responsibilities.

    That is a good suggestion, but is not always easy to get better pay job. :)



  • @RHuckster said:

    For teams as small as yours, I'd think you'd benefit more from some pair-programming. Especially when you're working on a particularly difficult and complex piece of your software, it's always good to have a second pair of eyes.

    This is good no matter how experienced or advanced the programmers are. I've pair-programmed with the CTO, who is competent and well-versed in software design, and have caught a few silly mistakes from him, which could have cost him a bit of troubleshooting in the future when he started testing it.

    Doing an hour or two of pair-programming every several days gives the benefit of introducing more than one programmer to the piece of software, improves communication between programmers, and can give each programmer some insight in the method of thought of the other. It's sort of like a code-review, only it's done as the code is being written, which IMO is more productive than having programmers get up from their keyboards and spend an hour or two NOT programming and only reviewing what they had done in previous days.

     

    I'll have to consider it. I never really thought about it before because I got burned on pair programming in college. We had a couple classes that required pair programming on all assignments, and basically I did 95% of the work while the other guy watched. If I tried to get the other guy coding usually he'd end up running the keyboard while I told him what to type.


  • 🚽 Regular

    @mott555 said:

    I'll have to consider it. I never really thought about it before because I got burned on pair programming in college. We had a couple classes that required pair programming on all assignments, and basically I did 95% of the work while the other guy watched. If I tried to get the other guy coding usually he'd end up running the keyboard while I told him what to type.

     

    Ah, but here's the way pair-programming works: Because there's only one keyboard, someone *does* have to type, while the other watches. Where I work, we take turns: one day I'll be the "programmer" while the other will be the "observer." The next time we do it, it'll be the other way around. Obviously it only works if the observer is attentive and is willing to actually contribute to the process by pointing out anything that he would do another way, and the programmer needs to be open to criticism (just as he does in a code-review). The observer could pick up a technique he never considered before, and the programmer could, too, if the observer says, "Here's what I would do..."

    Again, pair-programming works best if the task at hand is complicated. It's a waste of time if you're putting together a simple view or are coding something that is mundane. And it requires attention and cooperation from both programmers.

    http://en.wikipedia.org/wiki/Pair_programming



  • @Medezark said:

    @mott555 said:

    [ As far as I can tell, job titles around here are whatever you make up for yourself.



    Can I come work for you?  My Title would be Spanish Inquisitor of Code Review....
    You would, of course, have to provide the Comfy Chair.

    Ooo ooo, I wanna be Grand Poobah! Like in the Flintstones, it's obscure and awesome. Beats "Senior Web Analytics Engineer" any day.



  • @mott555 said:

    Yeah, I definitely understand that we're too small for formal stuff. On the other hand, a lot of lazy work (sometimes including my own) gets published right now and I'd like to see less of that. These past few weeks I've been trying to figure out what I can do to help out without locking our tiny team into some draconian processes that don't make sense for our size. Right now I think we need some way of reviewing each other's code so we can hold each other accountable to the coding standards we've come up with and improve each other's work, at least until everyone gets used to them. It's just painful when I have to fix a bug in someone else's code and I find one window is all over the variables and controls of another window or two instead of using some common data class that all those windows will access, or when I find the same 10-line code snippet copy-pasted in eight different places in the same class file.

    So, another question: How concerned should I be about that stuff? We have a working relatively non-WTF product, am I just being anal about how the code looks then?

    How about random, public code reviews? Schedule an hour in a conference room with a projector, pick a few source control check-ins at random, and go through the code line by line on the projector. Allow the audience to yell insults. (Keep it anonymous if you think feelings will be hurt.) Sure, you can't hit every line of shipped code that way, but you can ingrain some good processes and programmers can improve areas they're currently weak in.

    Or you could (shock, awe) just try a couple of these ideas and see what sticks. Try pair programming for a week, try the 1 hour meeting another week, ask your devs which they like better. I mean, if there's no one good answer-- try all the answers! As an added bonus, you get to look like a proactive leader who values your employee's opinions, instead of a dictator.



  • @RHuckster said:

    Because there's only one keyboard
     

    What if you're used to different keyboard layouts? That seriously hampers typing fluency.

    Also I can't code without my macros. Fuck if I'm going to write out console.log('') constantly whend debugging some code on another guy's computer.



  • @RHuckster said:

    @mott555 said:

    I'll have to consider it. I never really thought about it before because I got burned on pair programming in college. We had a couple classes that required pair programming on all assignments, and basically I did 95% of the work while the other guy watched. If I tried to get the other guy coding usually he'd end up running the keyboard while I told him what to type.

     

    Ah, but here's the way pair-programming works: Because there's only one keyboard, someone does have to type, while the other watches. Where I work, we take turns: one day I'll be the "programmer" while the other will be the "observer." The next time we do it, it'll be the other way around. Obviously it only works if the observer is attentive and is willing to actually contribute to the process by pointing out anything that he would do another way, and the programmer needs to be open to criticism (just as he does in a code-review). The observer could pick up a technique he never considered before, and the programmer could, too, if the observer says, "Here's what I would do..."

    Again, pair-programming works best if the task at hand is complicated. It's a waste of time if you're putting together a simple view or are coding something that is mundane. And it requires attention and cooperation from both programmers.

    http://en.wikipedia.org/wiki/Pair_programming


    Two keyboards.

    Two mice.

    One Computer.

    Profit.


  • Garbage Person

     @RaspenJho said:

    Two keyboards.
    Two mice.
    One Computer.
    Profit.
    I've actually been doing this lately. Two monitors, too. It's very effective - you can literally just leap in and go "OH I KNOW LET ME DO THIS BIT!" without having to shuffle seats around and lose your train of thought. And yeah, it shouldn't be used for the mundane bullshit we all do day in and day out, just on the exciting, exotic 'new' stuff. 



  • @Weng said:

    you can literally just leap in and go "OH I KNOW LET ME DO THIS BIT!"
     

    I never, never, ever have this feeling.

    So yeah, I stand by my case that PP is for social programmers only.



  • @dhromed said:

    @Weng said:

    you can literally just leap in and go "OH I KNOW LET ME DO THIS BIT!"
     

    I never, never, ever have this feeling.

    So yeah, I stand by my case that PP is for social programmers only.




    A social programmer? Are those the guys who program stuff like Facebook?



  • You should use a code review tool (and use it religiously).  Google's review tool interacts well with subversion, allows you to specify and notify reviewers and has decent support for commenting.

    Don't do code reviews in groups or in the old way, after code gets checked in, just bounce reviews of changes (even little ones) between you, the other programmer and the intern.  Wait for them to be OKd.  You'll be surprised what turns up.



  • @Rhywden said:

    A social programmer? Are those the guys who program stuff like Facebook?
     

    Programmers who don't mind being in each other's personal zone for hours and think out loud with jubilant diction.

    I hate people who think out loud.

    I also hate loud people period.



  • Everything the intern does should be checked.

    You and the other programmer should test each others work. As part of testing code review it.

    It's vital that you two guys follow the standards you've outlines otherwise the intern will develop your bad habits.


Log in to reply