Derik Whittaker

Syndication

News


Images in this post missing? We recently lost them in a site migration. We're working to restore these as you read this. Should you need an image in an emergency, please contact us at imagehelp@codebetter.com
Coding No - No's

Over time we all evolve as a coder, the code we write today is hopefully better then the code we wrote yesterday.  In turn, the code we write tomorrow should be better then the 'great code' we wrote today.  If this is not the case then we are not learning and evolving as professionals.

I have put together some coding no-no's that I remember encountering over time.

Multiple return paths in a method:
How often have we written a method where there are mulitple logic statements that would affect the value that is returned.  When all possible have one variable that acts as the return value holder and set that value in your logic and return it at the end of your method.  I know that there are exceptions to this, but as a general rule I feel this is the best way to go.

Good Example

Bad Example


Naming Confusion:
When we declare a parameter for a method we should make sure that the parameter name is not the exact (casing, spelling, etc) same as some local instance varable.  I know the compiler will know the correct scope, but it is not fun for the developer to have to debug.

Good Example

Bad Example



Finding objects based on Display Values:
When we have a list of objects in a collection, array, etc we should always be looking for them via some sort of key.  The key value can be anything as long as it is unique.  We should never lookup values based on some string values such as a name, or display value.  This will lead to poor results as well as very poor code to maintain and extend.

Good Example

Bad Example



Reuse of local variables with differnet meanings:
When we declare a local value name it so that the intent can be clear defined, but also only used it for its intent.  Don't create a variable that is resued in the method for different intents.

Good Example

Bad Example

I know there are more examples of coding No-No's these are just a few I could remember seeing in the past. 


Posted 05-18-2007 7:10 AM by Derik Whittaker
Filed under:

[Advertisement]

Comments

damieng wrote re: Coding No - No's
on 05-18-2007 9:33 AM

Totally disagree with "Multiple return methods" - the bad example is far easier to read and follow than the good example.

The "bad" example you can follow the flow and immediately know the return value and that no further processing will appear.  In the "good" example you have to read through the whole lot to determine the outcome.

Of course the "bad" example should simply have been:

 If ( whatever != null )   {  

        switch ( whatever.SomeEnum )   {  

            case 1:  

                return a;  

            case 2:  

                return b;  

            case 3:  

                return c;  

            case 4:  

                return d;  

            default:

                return GetAReallyMagicalValue();  

        }    

    }  

    else  {  

        return GetMagicalValue();  

    }  

Google for examples of failing fast - a principle where you test the various preconditions/parameters and return/throw before you start trying to process them.

The example you have regarding finding values is not a good one. Both examples are effectively looking for items by a key itterating over the whole collection.

This is not good for performance as it will suffer as the collection grows. You are much better off using a Dictionary<T,T> class instead of List<T> which allows you to access via a key and is internally implemented as a hash table for performance that will scale better.

[)amien

damieng wrote re: Coding No - No's
on 05-18-2007 9:39 AM

Of course a better version of the first example would be to handle the unusual case first:

If ( whatever == null )

 return GetMagicalValue();  

switch ( whatever.SomeEnum ) {

           case 1:  

               return a;  

           case 2:  

               return b;  

           case 3:  

               return c;  

           case 4:  

               return d;  

           default:

               return GetAReallyMagicalValue();  

}

Derik Whittaker wrote re: Coding No - No's
on 05-18-2007 10:08 AM

Damieng

In my opinion multiple return paths through code adds complexity.  It makes testing, debugging and enhancing harder in my opinion.  

I do agree with checking for failure cases up front, but in this example the case statement is not failure.  What if the case 3 and 4 did not do a return, it keep going though the method.  Makes reading more complicated.

If the method becomes too large then functional decomposition should be used to make life easier.

As for your dictionary comment, you are correct.  This code was only an example, not real world code.

Jim Bolla wrote re: Coding No - No's
on 05-18-2007 7:02 PM

This is all personal style, so to each his own, but I disagree with "Multiple return paths in a method". In your example, I think the good example is much harder to read than the bad example. My solution would be such:

public ISomething GetISomething(IWhatever whatever)  

{

   if (whatever != null)

       return GetISomething(whatever.SomeEnum);

   else

       return GetMagicalValue();  

}

protected virtual ISomething GetISomething(SomeEnum someEnum)  

{

   switch (someEnum)

   {

       case 1:  

            return a;  

       case 2:  

            return b;  

       case 3:  

            return c;  

       case 4:  

            return d;  

       default:

            // all enum possibilities should be covered by the base implementation explicitly

            throw new ArgumentOutOfRangeException();

}

By isolating the switch statement in its own method, it not only makes both methods more easily readable, but also allows for an inheriting class to override the second method, if such a thing would ever be desirable:

protected overrides ISomething GetISomething(SomeEnum someEnum)  

{

   switch (someEnum)

   {

       case 3:  

            return z;  

       default:

            return base.GetISomething(someEnum);

}

I guess my point is that the original method is actually doing too much, which necessitates the need for extra return paths. By splitting it into two methods, the return paths are kept closer together and make it much more understandable. Bonus buzzwords: Cyclomatic Complexity, Law of Demeter.

For "Naming confusion", Resharper changes the color coding so that fields are purple and parameters/variables are black, which makes this not such a big deal (for me anyways). I think the underscores are ugly. Normally for this case, for methods I try to make the parameter names more meaningful. In the case of constructors that are setting field values, I code it as "this.name = name;"

Tony wrote re: Coding No - No's
on 05-18-2007 9:44 PM

Naming Confusion --

I'm a big fan of the "this" keyword:

  this.someIntValue = someIntValue;

tells me exactly what's going on. Yes, it's four more characters, but to me looks looks a lot nicer that a bunch of underscores.

Then I don't have to worry about checking for duplicate names and worrying about side-effects when adding a new variable.

To me, the benefit of using "this" out-weighs the benefits of not using it. Once again, I guess it's personal preference.

Tony wrote re: Coding No - No's
on 05-18-2007 9:53 PM

Multiple Return Paths --

I'm voting with the other guys on this one. I have tried both ways over the years and think that multiple return paths are easier to read and maintain business logic in most cases -- but only for shorter sized functions (which are a majority).

For longer functions, I do prefer to use a single return like you suggested.

Derik Whittaker wrote re: Coding No - No's
on 05-19-2007 9:23 AM

All,

Looks like my multiple returns No-No is mostly personal opinion.  To me, single returns (with the exception of failure cases) are easier to read and debug, but looks like i am out voted on this one.

Oh well...

dudamir wrote re: Coding No - No's
on 05-21-2007 2:51 PM

I agree with "Multiple return paths in a method", specially in the context shown in the example.

I usually apply the Fail Fast principle, but not in the example context. The principle doesn't apply there because of its concept:

"where you test the various preconditions/parameters and return/throw before you start trying to process them."

One thing is test parameters and preconditions, early in the code. The other thing is multiple returns along the code and in different identation levels.

Viagra. wrote Generic viagra no prescription.
on 04-09-2009 10:59 AM

Free viagra.

About The CodeBetter.Com Blog Network
CodeBetter.Com FAQ

Our Mission

Advertisers should contact Brendan

Subscribe
Google Reader or Homepage

del.icio.us CodeBetter.com Latest Items
Add to My Yahoo!
Subscribe with Bloglines
Subscribe in NewsGator Online
Subscribe with myFeedster
Add to My AOL
Furl CodeBetter.com Latest Items
Subscribe in Rojo

Member Projects
DimeCasts.Net - Derik Whittaker

Friends of Devlicio.us
Red-Gate Tools For SQL and .NET

NDepend

SlickEdit
 
SmartInspect .NET Logging
NGEDIT: ViEmu and Codekana
LiteAccounting.Com
DevExpress
Fixx
NHibernate Profiler
Unfuddle
Balsamiq Mockups
Scrumy
JetBrains - ReSharper
Umbraco
NServiceBus
RavenDb
Web Sequence Diagrams
Ducksboard<-- NEW Friend!

 



Site Copyright © 2007 CodeBetter.Com
Content Copyright Individual Bloggers

 

Community Server (Commercial Edition)