Coffeehouse Thread

48 posts

Forum Read Only

This forum has been made read only by the site admins. No new threads or comments can be added.

Code Review 101

Back to Forum: Coffeehouse
  • User profile image
    ScanIAm

    I'm interested in what the rest of you would flag as a code review 'error' if you saw it in checked in code.

     

    For me:

     

    1) method comments.  We all forget, sometimes, but seriously.

    2) Too much work (a.k.a Too many notes (see: amadaeus)).  If the algorithm could be cleaner, it should be.  These code reviews are usually a way to teach developers a better way to do stuff.

    3) That's not your layer.  Just because you have access to a lower/higher layer, doesn't mean you should use the methods found there.

    4) Check your input.  If a paramenter can be null, check for it.  If a parameter is an entity and you expect that entity to be populuated with some data, check for it.

    5) I need a bigger screen.  If at all possible, break the algorithm up into smaller pieces. 

    6) Wow, so you copied the code from somewhere else.  If the source code was the current application, can you find a way to re-use what was already there.  If the source code was from codeproject/google/etc. could you clean it up so it looks like the rest of our code? 

    7) variable names.  I know it takes you less time to type smaller names, but the only time being saved is YOURS.  Everyone else hates you for this.  Name the variable for what it is or contains.

     

    That's a bit of a start, but I'd be interested to see what other kind of issues people find.

  • User profile image
    blowdart

    1 is caught by check-in policy (Gawd bless ya TFS)

    4 only on public methods. Or code contracts.

    7 check-in policy again Smiley

  • User profile image
    jmzl666

    blowdart said:

    1 is caught by check-in policy (Gawd bless ya TFS)

    4 only on public methods. Or code contracts.

    7 check-in policy again Smiley

    can you share the rules or a link on how to create that kind of policies? I'm configuring a new TFS2010 and I need a lot of that kind of polcies.

     

    Thanks.

     

  • User profile image
    blowdart

    jmzl666 said:
    blowdart said:
    *snip*

    can you share the rules or a link on how to create that kind of policies? I'm configuring a new TFS2010 and I need a lot of that kind of polcies.

     

    Thanks.

     

    http://code.msdn.microsoft.com/StyleCopPolicy

     

    I didn't do it mind you. Argument checks are a standard Code Analysis rule, and you can enforce those out of the box I believe.

  • User profile image
    Dr Herbie

    In addition:

     

    1. Magic numbers or strings -- create a constant with an appropriate name.

    2. Too many overloads -- do we really need 5 versions of the same method name? Might be clearer, perhaps, if they had different names to correctly reflect their purpose. Also suggests that the method has multiple responsibilities.

    3. Too many parameters (linked to 2).  More than three parameters suggests that either the method is too complex, or the method has more than one responsability.

    4. Boolean flags in parameters -- often suggests that method is doing two things. Additionally, it's hard to interpret the method calls; what does

    GetAccount(false)
    do differently from 
    GetAccount(true)
    ? Would be much better as 
    GetAccount()
    and 
    GetAccountWithoutContactHistory()
    .

    5. Lines too long -- I can't read it it smoothly if goes off the edge of the screen and I have to scroll.  This is sometimes used to try and make the method appear smaller.

     

    Herbie

     

     

     

  • User profile image
    kettch

    ScanIAm said:

    4) Check your input.  If A parameter can will be null, check for it.

     

    Fixed that for you. Trust but verify.

  • User profile image
    Bass

    Some things we look at in code reviews is method size. Very rarely does our codebase allow for methods longer then ~10 lines of code. Also, more important methods go to the top of the class and less important methods go to the bottom.

  • User profile image
    spivonious

    Code review? What's that? Wink Seriously though, our official code review has items like "commented sufficiently" and "adequate error handling". Nothing concrete, everything subjective.

     

    Things I've learned to check in my own code:

    1. lots of comments on any "hack"

    2. XML comments on every public member

    3. no implicit type conversions (Option Explicit FTW)

    4. no magic numbers

    5. magic strings are okay if they're only used once

    6. variable names and method names should be clear enough that comments aren't needed

    7. if I can't do anything about it, don't catch the exception

  • User profile image
    Ion Todirel

    you look at EVERYTHING, don't be afraid to be subjective, it's a code review....

  • User profile image
    kettch

    Ion Todirel said:

    you look at EVERYTHING, don't be afraid to be subjective, it's a code review....

    Even if you can't fix EVERYTHING put a TODO (or whatever your internal process uses) on lower priority things with a clear explanation of what needs to be done. Maybe, it'll get taken care of someday, and at the very least it'll be documented.

  • User profile image
    exoteric

    Dr Herbie said:

    In addition:

     

    1. Magic numbers or strings -- create a constant with an appropriate name.

    2. Too many overloads -- do we really need 5 versions of the same method name? Might be clearer, perhaps, if they had different names to correctly reflect their purpose. Also suggests that the method has multiple responsibilities.

    3. Too many parameters (linked to 2).  More than three parameters suggests that either the method is too complex, or the method has more than one responsability.

    4. Boolean flags in parameters -- often suggests that method is doing two things. Additionally, it's hard to interpret the method calls; what does

    GetAccount(false)
    do differently from 
    GetAccount(true)
    ? Would be much better as 
    GetAccount()
    and 
    GetAccountWithoutContactHistory()
    .

    5. Lines too long -- I can't read it it smoothly if goes off the edge of the screen and I have to scroll.  This is sometimes used to try and make the method appear smaller.

     

    Herbie

     

     

     

    1. Don't use constants, use enums.

     

    4. Don't [necessarily] use two methods. Do use named arguments.

     

    GetAccount(withContactHistory : true)

     

    Named arguments used in this way is a way to get the benefits of enums without actually creating an enum.

  • User profile image
    exoteric

    4. Contract.Requires(arg != null); or better yet: X Op(A! arg) ...

    7. It's good to name class instance variables decently, as in self-describing, to a point. It is much less important to create long method/function-local variable names, in my oppinion; e.g. g or gc for graphics context, aso. Parameter names should also be self-describing, to a point.

  • User profile image
    blowdart

    exoteric said:
    Dr Herbie said:
    *snip*

    1. Don't use constants, use enums.

     

    4. Don't [necessarily] use two methods. Do use named arguments.

     

    GetAccount(withContactHistory : true)

     

    Named arguments used in this way is a way to get the benefits of enums without actually creating an enum.

    Blindly replacing consts with enums is a bit harsh (and limiting - int/long only)

  • User profile image
    Ion Todirel

    exoteric said:

    4. Contract.Requires(arg != null); or better yet: X Op(A! arg) ...

    7. It's good to name class instance variables decently, as in self-describing, to a point. It is much less important to create long method/function-local variable names, in my oppinion; e.g. g or gc for graphics context, aso. Parameter names should also be self-describing, to a point.

    Doesn't matter man, just teach the cop and be consistent, or rather respect it's opinion Big Smile I would argue any of those are based on personal preference, of course it's not always like that, we all know prefixing the local variables with "_" is the right thing to do Wink

  • User profile image
    blowdart

    Ion Todirel said:
    exoteric said:
    *snip*

    Doesn't matter man, just teach the cop and be consistent, or rather respect it's opinion Big Smile I would argue any of those are based on personal preference, of course it's not always like that, we all know prefixing the local variables with "_" is the right thing to do Wink

    Right, time to go up your management chain sending emails ....

  • User profile image
    Dr Herbie

    exoteric said:
    Dr Herbie said:
    *snip*

    1. Don't use constants, use enums.

     

    4. Don't [necessarily] use two methods. Do use named arguments.

     

    GetAccount(withContactHistory : true)

     

    Named arguments used in this way is a way to get the benefits of enums without actually creating an enum.

    "4. Don't [necessarily] use two methods. Do use named arguments."

     

    I haven't got to v4 yet, only just got to 3.5 Sad

     

    Herbie

  • User profile image
    exoteric

    blowdart said:
    exoteric said:
    *snip*

    Blindly replacing consts with enums is a bit harsh (and limiting - int/long only)

    Let's add this rule then: don't use enums when you can't! Wink

    PS - don't replace: don't make them in the first place Wink

  • User profile image
    ScanIAm

    Dr Herbie said:

    In addition:

     

    1. Magic numbers or strings -- create a constant with an appropriate name.

    2. Too many overloads -- do we really need 5 versions of the same method name? Might be clearer, perhaps, if they had different names to correctly reflect their purpose. Also suggests that the method has multiple responsibilities.

    3. Too many parameters (linked to 2).  More than three parameters suggests that either the method is too complex, or the method has more than one responsability.

    4. Boolean flags in parameters -- often suggests that method is doing two things. Additionally, it's hard to interpret the method calls; what does

    GetAccount(false)
    do differently from 
    GetAccount(true)
    ? Would be much better as 
    GetAccount()
    and 
    GetAccountWithoutContactHistory()
    .

    5. Lines too long -- I can't read it it smoothly if goes off the edge of the screen and I have to scroll.  This is sometimes used to try and make the method appear smaller.

     

    Herbie

     

     

     

    ah-HA.  I knew I forgot something.

     

    We have a sliding scale of magic values which tends to annoy new developers.

     

    If you have a one-off function that needs to choose how to operate based on the values in an entity, then likely, I won't code review it, but it had better be defined in a constants class, and it had better be named appropriately.  I'll admit that this is slackness on my part.

     

    If the check for that value can be reached by 2 or more calls, then you've found something that needs to become part of the data store. 

     

    That said, we're a bit lax on the the string constants, but we expect strings to be in the business layer and they'd better be defined as constants.  Further, NO STRING OUTPUT without defining either the output, or the format as a constant, and RegEx's can never be method local.

     

    I'll admit that it makes debugging a bit of a pain, and I've been bombarded with lazy devs asking me to lighten up, but it has resulted in cleaner code and it's forced them to think a bit more about what they do.

     

     

Conversation locked

This conversation has been locked by the site admins. No new comments can be made.