Code Shock

17-Aug-10 12:22 am EST Leave a comment Go to comments

I call it “code shock”.  Every so often I stumble into some code that at least at the outset defies all attempts at understanding.  And, I am perfectly willing to admit — I don’t know everything.  (Note to recruiters without a strong technical background who take the time to read my blog entries: I’d argue that being able to say that is a good thing!)  Yet despite the suspicion I’m missing some brilliant approach that I could leverage myself at a later date, the following method made me do a double-take recently: 

    8     public class Behaviours

    9     {

   10         private string name;

   11         private Type type;

   12 

   13         public void GetResult(string name, Type type)

   14         {

   15             if (name == null) { throw new ArgumentException("A name must be provided."); } 

   16             if (type == null) { throw new ArgumentException("A type must be provided."); }

   17             name = name;

   18             type = type;

   19         }

   20 

   21         public string Name

   22         {

   23             get { return name; }

   24             set { name = value; }

   25         }

   26 

   27         public Type Type

   28         {

   29             get { return type; }

   30             set { type = value; }

   31         }

   32     }

 

The problem, of course, lies on lines 16 and 17 in the above listing.  The method these lines belong to have accepted parameters called “name” and “type” above on line 13; yet the developer who implemented this saw no issue assigning the values to themselves by using the same names.  Since “name” and “type” already equal themselves, respectively — the logic is redundant and, indeed, the C# compiler throws a CS1717 warning (“Assignment made to same variable; did you mean to assign something else?”).

There are a number of other issues with this class too.  Indeed, it might be an example worthy of presentation to a class of newbie C# students or perhaps interview material for a junior programmer.  In any case, it’s certainly not worthy of production code and, hopefully, is merely the result of someone somehow using either a find/replace or other code automation within Visual Studio and missing the fact that they’ve got two private members in the class of the same name which won’t get assigned because the parameters will simply overwrite themselves used this way.

Another fun pattern that was uncovered on the same occasion as this gem was the more familiar anomaly….the anonymous re-thrown exception.  It looks something like this in C#:

   34     public static class SampleExceptions

   35     {

   36         public static void MyException(Exception sampleEx)

   37         {

   38             try

   39             {

   40                 int x = 10;

   41                 int y = 0;

   42                 int z = x / y;

   43 

   44             }

   45             catch

   46             {

   47                 throw;

   48             }

   49         }

   50     }

 

As you might have guessed from lines 45 through 48, the static method MyException(Exception) is gonna throw a division-by-zero exception; but since it’s technically handled by a catch (error handling) block an unspecified exception will percolate “upwards” — back to the caller.  Some developers make the mistaken assumption that error reporting is unaffected using this approach, but in fact checking-in such code makes troubleshooting all the more difficult if anything ever goes wrong here.  Consider the stack traces returned by this block and other, properly-structured try-catch block:

Unhandled Exception: System.DivideByZeroException: Attempted to divide by zero.
   at RossReport.Samples.BadEquivalency.SampleExceptions.MyException(Exception sampleEx) in C:\Users\holderr\My Code\BadEquivale
ncySample\BadEquivalencySample\Behaviours.cs:line 49
   at RossReport.Samples.BadEquivalency.Program.Main(String[] args) in C:\Users\holderr\My Code\BadEquivalencySample\BadEquivale
ncySample\Program.cs:line 18

The exception cited above was unhandled, which is bad, yes. But you’ll also note that it claims the failure point is line 49 on which the only logic visible to us is a little close curly brace character (“}”).  In other words, our stack trace is useless because on unhandled exceptions of this sort, the .NET Framework has no means of reporting where the error occurred and one is left to but guess at where the wrench, much less the monkey went.

A properly-written catch block with at least a Debug or Trace statement serving to output the exception message and/or stack trace to a file or even the console window is much more useful:

   46             catch (Exception ex)

   47             {

   48                 Trace.WriteLine(String.Format("***** Exception occurred: {0}", ex.Message));

   49                 Trace.WriteLine(String.Format("STACK TRACE:\r{0})", ex.StackTrace));

   50             }

…gives us:

***** Exception occurred: Attempted to divide by zero.
   at RossReport.Samples.BadEquivalency.SampleExceptions.MyException(Exception sampleEx) in C:\Users\holderr\My Code\BadEquivale
ncySample\BadEquivalencySample\Behaviours.cs:line 43)

NOTE: The above includes an appropriately-configured trace listener in the .config file of the project hosting your code.

An even better and more popular solution for exception handling in all sorts of applications can be found in the Logging Application Block of the Microsoft Patterns & Practices Enterprise Library (a.k.a. EntLib), which is available on CodePlex.

(Strangely enough, the application I mentioned was the inspiration for this article uses the EntLib extensively….)

And apologies if anyone finds a sanctimonious subtext to my commentary here; the purpose of the article isn’t to malign anyone’s coding skills, God knows.  (Such was not my intent even when citing a couple of unnamed developers who found themselves stymied when entering a dot on a blank line in the VB editor didn’t summon intellisense….an activity that normally requires one be within a “With” clause.)  Instead, it’s a sincere effort to help fellow developers avoid causing their peers unnecessary cases of “code shock”; especially when delivery deadlines loom.

Happy coding!

P.S. You can the sample project used to create source code for this article here.

Advertisements
  1. No comments yet.
  1. No trackbacks yet.

Leave a Reply

Fill in your details below or click an icon to log in:

WordPress.com Logo

You are commenting using your WordPress.com account. Log Out / Change )

Twitter picture

You are commenting using your Twitter account. Log Out / Change )

Facebook photo

You are commenting using your Facebook account. Log Out / Change )

Google+ photo

You are commenting using your Google+ account. Log Out / Change )

Connecting to %s

Terry Glavin

CHRONICLES

Techno Manor

Geek's Corner

VM.Blog.

an IT blog.. and an occasional rant

Yammer Site Status

Is Yammer down? Offline? Broken? Undergoing scheduled maintenance? When will it be back? Find out here.

jalalaj

A journey full of wonderful experiences

Azure and beyond

My thoughts on Microsoft Azure and cloud technologies

TechCrunch

Startup and Technology News

Ottawa Citizen

Ottawa Latest News, Breaking Headlines & Sports

National Post

Canadian News, World News and Breaking Headlines

Targeted individuals's

One Government to rule them all.

Joey Li's IT Zone

Everything about IT

jenyamatya

Unravelling the magik of code...

The Bike Escape

Because Cycling is Life

The Ross Report

Now you know where you need to know more...

Lights in the Dark

A journal of space exploration

Strength Rehabilitation Institute

Bridging the gap between physiotherapy and exercise.

The Ross Report

Now you know where you need to know more...

Little Girl's Mostly Linux Blog

Nothing to see here. Move along...

David Eedle

Geek, tech, programmer, business owner. Serial starter of things. Occasional finisher. Oh, and please don't call me Dave.

%d bloggers like this: