Archive

Archive for the ‘Refactoring’ Category

Can you find the bug in this code? (THE FIX)

March 26, 2008 2 comments

Thanks to everyone for contributing!  It was really neat to read everyone’s ideas and see the discussion and review (talking about code is always fun!).  Here is a summary of responses and the “fixed” code. 

If you are interested in the original problem, go here.

 

@drakiula: The idea with the Response.Redirect is that it will stop processing so the else is not needed on line 23.

@JV: Yes, this code could definitely be refactored! 🙂
And your suggestion would squish the bug.

@rajbk: Nice. Yes, the ThreadAbortException is definitely part of the bug but I don’t think the Redirect(url, false) is the best way to go.

@Basgun: Changing line 17-23 as you suggest would make it cleaner but wouldn’t fix the bug.

@cibrax,Lee:  Nice. Yes, the ThreadAbortException will cause the catch to fire everytime and redirect the user to the same page.

@Thomas Eyde: Nice. Yes, the refactoring into separate responsibilities would make it much cleaner and also would somewhat inadvertently fix the bug since the redirect would happen outside the catch.

@Zack Jones:  Nice suggestions.  Thanks!

@Hosam Kamel: Yes, good point – there are several ways those method calls could fail (lots of encryption and authentication done within them) so the catch is there as a simple default “then send them here”.

Summary:

If you call Response.Redirect(string) or Response.Redirect(string, true), it will throw a ThreadAbortException.  So if you do the redirect inside a generic try/catch then your catch will fail everytime.  As pointed out by several people above, the code could be refactored so the responsibilities are separated out – this means that the Redirect should probably only be done in one place which would mitigate the issue.

I was surprised to see that Response.Redirect throws this exception especially since this exception is not thrown up the Application_Error event so some “magic” must happen within ASP.NET to issue the HTTP location header but not allow the exception to continue as a real exception.  This seems like a misuse of exceptions since a Response.Redirect is hardly an “exceptional” case and in many applications happens somewhere on every page!
That said, it is reasonable to see how Microsoft uses this mechanism to prevent further execution in the page.

Either way, it is a good gotcha to remember!

Here is the “fixed” code:  (although this could still be seriously refactored)

01: private void Page_Load(object sender, EventArgs e)
02: {
03: string redirectUrl = null;
04: try

05: {

06:     AuthenticationInfo authenticationInfo = GetAuthenticationInfo(Request.QueryString[“t”]);

07:     Authenticator authenticator = new Authenticator(new LoginProvider());

08:     AuthenticationStatus authenticationStatus = authenticator.Authenticate(authenticationInfo);

09:     if (authenticationStatus.Authenticated)

10:     {

11:         IUser user =

12:             BLUser.Load(authenticationInfo.UserName, authenticationInfo.OrganizationCode, aut
henticationInfo.DomainId);

13:         user.SetNewSessionId();

14:         AuthenticationTokenParser authenticationTokenParser = new AuthenticationTokenParser();

15:         string authenticationToken = authenticationTokenParser.Create(user.UserName, user.OrganizationId,

user.DomainId);

16:         FormsAuthentication.SetAuthCookie(authenticationToken, true);

04:         redirectUrl = FormsAuthentication.GetRedirectUrl(authenticationToken, true);

04:     }

16: }

17: catch

18: {

19:     // If anything goes wrong (encryption error, etc) then just let it fall through to the redirect.

20: }

21: if (redirectUrl == null || redirectUrl.Trim().Length == 0)

22: {

23:     redirectUrl = “~/Home.aspx”;

24: }

25: // NOTE: If you do the Redirect inside the try/catch it will get a ThreadAbortException!

26: Response.Redirect(redirectUrl, true);

27: }

 

We are hiring!  Do you want to write beautiful code in a Test Driven, Refactored, Agile .NET software company in the heart of Washington DC and work on cool products
Take the code test and send your resume along with why you want to join Thycotic to
tddjobs@thycotic.com.

Please question the need for whitespace

March 18, 2008 13 comments

I have blogged about this before but I think it is a common problem that is worth restating since it affect developers across our industry.  I noticed the following method recently and again the curious separation of sections by whitespace popped into my head:

   1:  private void CalcHeaderOffsets()
   2:  {
   3:      this.fs = new FileStream( assemblyPath, FileMode.Open, FileAccess.Read );
   4:      this.rdr = new BinaryReader( fs );
   5:      dos_magic = rdr.ReadUInt16();
   6:      if ( dos_magic == 0x5a4d )
   7:      {
   8:          fs.Position = 0x3c;
   9:          peHeader = rdr.ReadUInt32();
  10:          fileHeader = peHeader + 4;
  11:          optionalHeader = fileHeader + 20;
  12:          dataDirectory = optionalHeader + 96;
  13:          // dotNetDirectoryEntry = dataDirectory + 14 * 8;
  14:   
  15:          fs.Position = peHeader;
  16:          pe_signature = rdr.ReadUInt32();
  17:          rdr.ReadUInt16(); // machine
  18:          numDataSections = rdr.ReadUInt16();
  19:          fs.Position += 12;
  20:          optionalHeaderSize = rdr.ReadUInt16();
  21:          dataSections = optionalHeader + optionalHeaderSize;
  22:   
  23:          sections = new Section[numDataSections];
  24:          fs.Position = dataSections;
  25:          for( int i = 0; i < numDataSections; i++ )
  26:          {
  27:              fs.Position += 8;
  28:              sections[i].virtualSize = rdr.ReadUInt32();
  29:              sections[i].virtualAddress = rdr.ReadUInt32();
  30:              uint rawDataSize = rdr.ReadUInt32();
  31:              sections[i].fileOffset = rdr.ReadUInt32();
  32:              if ( sections[i].virtualSize == 0 )
  33:                  sections[i].virtualSize = rawDataSize;
  34:   
  35:              fs.Position += 16;
  36:          }
  37:      }
  38:  }

.csharpcode, .csharpcode pre
{
font-size: small;
color: black;
font-family: consolas, “Courier New”, courier, monospace;
background-color: #ffffff;
/*white-space: pre;*/
}
.csharpcode pre { margin: 0em; }
.csharpcode .rem { color: #008000; }
.csharpcode .kwrd { color: #0000ff; }
.csharpcode .str { color: #006080; }
.csharpcode .op { color: #0000c0; }
.csharpcode .preproc { color: #cc6633; }
.csharpcode .asp { background-color: #ffff00; }
.csharpcode .html { color: #800000; }
.csharpcode .attr { color: #ff0000; }
.csharpcode .alt
{
background-color: #f4f4f4;
width: 100%;
margin: 0em;
}
.csharpcode .lnum { color: #606060; }

What is the purpose of the new lines at 14, 22 and 34?  Are these separate blocks of logic?  Could they be better represented using “Extract Method“?

One of the first things that I do with code when refactoring is remove all the line breaks and make sure I am comfortable with the logical flow of the code.  If lines don’t logically flow together then I ask myself if the two pieces belong next to each other or if they should be different responsibilities in different methods.

Ask yourself next time you leave a new line whether it is really necessary or is it a codesmell indicating poorly separated unrelated functionality?

FYI:  The code above is taken from NUnit (AssemblyReader.cs) and is free software licensed under the NUnit license (Copyright 2007, Charlie Poole, http://nunit.org/?p=license&r=2.4).

 

We are hiring!  Do you want to write beautiful code in a Test Driven, Refactored, Agile .NET software company in the heart of Washington DC and work on cool products
Take the code test and send your resume along with why you want to join Thycotic to
tddjobs@thycotic.com.

NoVa CodeCamp South v1 speaking schedule has been announced!

March 8, 2008 Leave a comment

novacodecampsouth The NoVa CodeCamp South v1 will be held on March 29th 2008 in Woodbridge VA.  The speaker schedule has been posted here.

I will be presenting on two topics:

  • Refactoring in C# – bad code to better code
  • Web Application Testing in Watin

There are lots of great sessions from WPF, TDD, SSIS, jQuery, SQL Server 2008 and more…

You can register now.

 

We are hiring!  Do you want to write beautiful code in a Test Driven, Refactored, Agile .NET software company in the heart of Washington DC and work on cool products
Take the code test and send your resume along with why you want to join Thycotic to
tddjobs@thycotic.com.

Refactoring in C# at RockNUG this week

March 8, 2008 Leave a comment

I will be giving a presentation on Refactoring in C# at RockNUG on Wednesday March 12th 2008 at 6:30pm.  Directions here.

What could be more fun on a Wednesday evening than critiquing some bad code and making it better? 🙂 Come along to learn how to clean code like the Thycotic team. What do we look for? How do we take small steps to keep it working? What tips and tricks make it easier? This session will be code, code and more code (and a few unit tests of course!).

Picking over some code and discussing it is so much fun.  Come along and join us …

 

We are hiring!  Do you want to write beautiful code in a Test Driven, Refactored, Agile .NET software company in the heart of Washington DC and work on cool products
Take the code test and send your resume along with why you want to join Thycotic to
tddjobs@thycotic.com.

What makes some code confusing?

February 23, 2008 18 comments

Developers look at code for hour upon hour every day.  After some years of doing this, you can just look at something and almost intuitively understand what it is doing – assuming that some effort has been made by the developers to keep the code clear and understandable.  But every now and then, you find a doozy.

I came across this one while working on a feature with Alessandro – my programming pair partner that day.

public bool MayUserChangeEntity()
{
    return !(Id > 0 && Security.GetCurrentAccount().IsNormalUser() && Entity.GetEntity(EntityId).EntityNumber > 0);
}

I looked at this with my pair for a good five minutes and was still no closer to grasping it!  Why do I struggle to understand this expression?

  • Because it is all on one line?  No, ternaries are perfectly normal and effective when used well.
  • Clearly some time had been spent by the developers to summarize the intent of this method and unfortunately the developers were taking advantage of the && operator to avoid executing the more expensive checks by putting them at the end (this prevented them using Introduce Explaining Variable since the performance gain would be lost).
  • Is it the number of terms being evaluated?  That seems unlikely since they are not overly complex.
  • Is it the negative return?  Yes, but more than that.  As the number of conditions increase it seems to be harder for me to understand the implication of the negative especially combined with the multiple “and” logic.  I think the brain struggles a little with the negation and the boolean && … you start wondering if all the && become || due to the negation or is that incorrect?

Needless to say, we decided to “refactor to understand” and quickly broke it out into simpler terms:

public bool MayUserChangeEntity()
{
    return !IsSaved() || Security.GetCurrentAccount().IsSpecialUser() || IsEntityDraft();
}

private bool IsSaved()
{
    return Id > 0;
}

private bool IsEntityDraft()
{
    return Entity.GetEntity(EntityId).EntityNumber == 0;
}

Let’s review what we achieved:

  • We kept the same basic structure of the conditions.
  • We created more methods (and more code) but made some of the expressions easier to understand by using a method name to explain the idea.  (Using Extract Method instead of Introduce Explaining Variable can be a good way to get clarity but still get the benefits of only evaluating the expression if necessary.)
  • We removed the negated boolean logic and converted the conditions to separate positive checks.

It is difficult to say that the resulting code is *MUCH* better but I have no trouble reading the one liner ternary now.  The human brain is a strange machine.

 

 

We are hiring!  Do you want to write beautiful code in a Test Driven, Refactored, Agile .NET software company in the heart of Washington DC and work on cool products
Take the code test and send your resume and why you want to join Thycotic to
tddjobs@thycotic.com.

Speaking on Refactoring at RockNUG in March

January 25, 2008 Leave a comment

I will be presenting on Refactoring in C# at the Rockville .NET User Group (RockNUG) on March 12th 2008 at 6:30pm.

Refactoring in C# – Bad code to better code
What could be more fun on a Wednesday evening than critiquing some bad code and making it better? 🙂 Come along to learn how to clean code like the Thycotic team. What do we look for? How do we take small steps to keep it working? What tips and tricks make it easier? This session will be code, code and more code (and a few unit tests of course!).

I always like Refactoring sessions because they give great opportunities for discussion about coding practices and also get lots of different opinions on how to improve the code.  I will be using Visual Studio 2005 and Resharper 3.0.

 

Jonathan Cogley is the CEO and founder of Thycotic Software, a .NET consulting company and ISV in Washington DC.  Our product, Secret Server is a enterprise password manager system for teams to secure their passwords.  Is your team still storing passwords in Excel?

Removing dead code

December 5, 2007 6 comments

Ambulance_Stretcher What does your code terrain look like?  Are there bodies of dead logic lying here and there?  Maybe they helped briefly while you worked towards a better solution or perhaps they just fell victim to changing business rules.

At a recent Code Camp, there was a question about code generation and I answered that we (as developers) are required to love every line of code (it drew a laugh from many).  But love isn’t cheap … it takes time, care and hard work.  Therefore we should only love and care for the lines of code that we absolutely need.  Anything not being used should be removed.

 

 

 

 

 

What excuses are there to keep unused code?

  • We may need it in the future
    • The classic argument – the typical agile response being YAGNI
    • Keeping it in the code-base takes maintenance in the form of understanding it, updating associated tests and working around it to add other functionality.
    • It will always remain in source control should we ever need it again
  • If it ain’t broke, don’t fix it.
    • Bob Martin sums this up well when he defines what “broke” means:  Code should 1) work 2) be maintainable and 3) communicate its intent.  Clearly dead code violates 2 and 3.
    • This idea also defeats the goal of loving every line of code – leaving something alone for no good reason is neglect.

I just spent a good half hour cleaning up some code and mostly removing unused code.  It was annoying since it wasn’t even being used but at least I am happier with the code now.  A good tool when investigating code you suspect may be dead is ALT-F7 using Jetbrains Resharper – you can easily find all usages of a method or class even in large code-bases.  A cheap alternative is to use “lean on the compiler” (as discussed by Michael Feathers) – this is simply changing a class name or method name by one character and try to compile (the compiler will then let you know what is depending on that code).

Jonathan Cogley is the CEO and founder of Thycotic Software, a .NET consulting company and ISV in Washington DC.  Our product, Secret Server is a enterprise password manager system for teams to secure their passwords.  Is your team still storing passwords in Excel?