Less Comments, More Readable Code #2

An entry about refactoring | clean code Publication date 3. July 2009 14:23

In my last post, I refactored some horribly commented code into the following:

public void HighlightPotentialDropNode(
                     UltraTreeNode potentialDropNode, 
                     Point dropLocationRelativeToTree)

{

    DropLinePositionEnum newDropLinePosition = DropLinePositionEnum.OnNode;

 

    if (PointIsAboveNode(potentialDropNode, dropLocationRelativeToTree))

    {

        newDropLinePosition = DropLinePositionEnum.AboveNode;

    }

    else if (PointIsBelowNode(potentialDropNode, dropLocationRelativeToTree))

    {

        newDropLinePosition = DropLinePositionEnum.BelowNode;

    }

 

    DrawDropLinePosition(potentialDropNode, newDropLinePosition);

}

 

private bool PointIsAboveNode(UltraTreeNode node, Point pointInTreeCoords)

{

    return pointInTreeCoords.Y < (node.Bounds.Top + EdgeSensitivity);

}

 

private bool PointIsBelowNode(UltraTreeNode node, Point pointInTreeCoords)

{

    return pointInTreeCoords.Y > ((node.Bounds.Bottom - EdgeSensitivity) - 1);

}

I was a bit quick to publish that post – because 5 minutes later I discovered that I wasn’t quite done cleaning up this method... Can you tell what’s wrong with it? It’s breaking the single responsibility principle.

Let’s do one more ‘extract method’ refactoring on it, putting the logic of calculating the DropLinePosition in a separate method:

public void HighlightPotentialDropNode(
                        UltraTreeNode potentialDropNode, 
                        Point dropLocationRelativeToTree )

{

    DropLinePosition dropLinePosition = 
                CalculateDropLinePosition(potentialDropNode, dropLocationRelativeToTree);

    DrawDropLinePosition(potentialDropNode, dropLinePosition);

}

 

private DropLinePosition CalculateDropLinePosition(
                                   UltraTreeNode potentialDropNode, 
                                   Point dropLocationRelativeToTree)

{

    DropLinePosition dropLinePosition = DropLinePosition.OnNode;

 

    if (DropLocationIsAboveNode(potentialDropNode, dropLocationRelativeToTree))

    {

        dropLinePosition = DropLinePosition.AboveNode;

    }

    else if (DropLocationIsBelowNode(potentialDropNode, dropLocationRelativeToTree))

    {

        dropLinePosition = DropLinePosition.BelowNode;

    }

 

    return dropLinePosition;

}

 

private bool DropLocationIsAboveNode(UltraTreeNode node, Point dropLocationRelativeToTree)

{

    return dropLocationRelativeToTree.Y < (node.Bounds.Top + EdgeSensitivity);

}

 

private bool DropLocationIsBelowNode(UltraTreeNode node, Point dropLocationRelativeToTRee)

{

    return dropLocationRelativeToTRee.Y > ((node.Bounds.Bottom - EdgeSensitivity) - 1);

}

Now we’re talking :)

Currently rated 4.0 by 1 people

  • Currently 4/5 Stars.
  • 1
  • 2
  • 3
  • 4
  • 5

Less Comments, More Readable Code

An entry about clean code | refactoring Publication date 3. July 2009 13:49

I’m currently implementing a drag and drop sorting feature for a tree view in a Windows Forms application. We’re using Infragistics UI controls, and their UltraTree doesn’t have drag and drop functionality out of the box. They do however have a sample in their SDK which explains how to implement it. Here’s an excerpt of that sample code – a method which determines where to draw a marker which tells the user where in the tree he’s about to drop (ie before or after the node the mouse pointer hovering):

public void SetDropHighlightNode(UltraTreeNode Node, Point PointInTreeCoords )

{

     //The distance from the edge of the node used to

     //determine whether to drop Above, Below, or On a node

     int DistanceFromEdge;

 

     //The new DropLinePosition

     DropLinePositionEnum NewDropLinePosition;

 

     DistanceFromEdge = mvarEdgeSensitivity;

     //Check to see if DistanceFromEdge is 0

     if (DistanceFromEdge == 0)

     {

         //It is, so we use the default value - one third.

         DistanceFromEdge = Node.Bounds.Height / 3;

     }

 

     //Determine which part of the node the point is in

     if (PointInTreeCoords.Y < (Node.Bounds.Top + DistanceFromEdge))

     {

         //Point is in the top of the node

         NewDropLinePosition = DropLinePositionEnum.AboveNode;

     }

     else

     {

         if (PointInTreeCoords.Y > ((Node.Bounds.Bottom - DistanceFromEdge) - 1))

         {

             //Point is in the bottom of the node

             NewDropLinePosition = DropLinePositionEnum.BelowNode;

         }

         else

         {

             //Point is in the middle of the node

             NewDropLinePosition = DropLinePositionEnum.OnNode;

         }

     }

 

     //Now that we have the new DropLinePosition, call the

     //real proc to get things rolling

     SetDropHighlightNode(Node, NewDropLinePosition);

}

Disregarding the clearly redundant comments (NewDropLinePosition is “the new DropLinePosition”?   phew, I’d never have guessed!), there are some that you might think deserve to stay – the ones that tell you what the if/else conditions are checking for, for instance.

However, take a look at my refactored version of this method. It has ZERO comments – yet I would argue that it is much easier to understand:

public void HighlightPotentialDropNode(
                     UltraTreeNode potentialDropNode, 
                     Point dropLocationRelativeToTree)

{

    DropLinePositionEnum newDropLinePosition = DropLinePositionEnum.OnNode;

 

    if (PointIsAboveNode(potentialDropNode, dropLocationRelativeToTree))

    {

        newDropLinePosition = DropLinePositionEnum.AboveNode;

    }

    else if (PointIsBelowNode(potentialDropNode, dropLocationRelativeToTree))

    {

        newDropLinePosition = DropLinePositionEnum.BelowNode;

    }

 

    DrawDropLinePosition(potentialDropNode, newDropLinePosition);

}

 

private bool PointIsAboveNode(UltraTreeNode node, Point pointInTreeCoords)

{

    return pointInTreeCoords.Y < (node.Bounds.Top + EdgeSensitivity);

}

 

private bool PointIsBelowNode(UltraTreeNode node, Point pointInTreeCoords)

{

    return pointInTreeCoords.Y > ((node.Bounds.Bottom - EdgeSensitivity) - 1);

}

Apart from renaming stuff to give them more meaningful names, notice particularly how I’ve used extract method on the predicates of the if statements, giving them names which clearly state what the conditions are. This has an important effect: All the code in this method is now at the same level of abstractionand this makes it much more readable.

Extract method is probably the simplest and most effective refactoring tool in your arsenal. Use it lavishly! If you want to learn more about writing clean functions, I suggest you take an hour to watch Robert C. Martin’s great “Clean Code: Functions” talk – here’s a recording from NDC 2009 (see more NDC recordings here).

Update: read the follow-up to this post here.

Currently rated 5.0 by 1 people

  • Currently 5/5 Stars.
  • 1
  • 2
  • 3
  • 4
  • 5

Powered by BlogEngine.NET 1.4.5.0

Welcome!

My name is Fredrik Kalseth, and this is my blog - thanks for visiting! I am fortunate enough to work with what I love for a living, and this blog is essentially the biproduct of that.

I work as a senior consultant for Capgemini, and am also an active participant in the Norwegian .NET community, as an avid attendee but also as a speaker (most recently at NNUG and MSDN Live).

As a developer, I have a wide circle of interest. My primary passion is for agile, test-driven development, with focus on best practices and clean code. That said, I also love to work on the frontend, especially with web development.

On Twitter? My handle is fkalseth. On LinkedIn? I`m there too.

NDC 2010

The conference to attend this summer happens June 16th-18th in Oslo, Norway. Are you going? Be sure to catch my talk on AOP while you're there!

 

Disclaimer

This is a personal blog; any opinions expressed here are my own and do not necessarily reflect those of my employer. All content herein is my own original creation, and as such is protected by copyright law. Unless otherwise stated, all source code posted on this blog is freely usable under the Microsoft Permissive License.

What Readers Talk About

Comment RSS