Less Comments, More Readable Code

最新推荐文章于 2025-12-18 16:14:31 发布

转载最新推荐文章于 2025-12-18 16:14:31 发布 · 719 阅读

文章标签：

#comments #less #refactoring #sorting #distance #tree

c/c++/C# 专栏收录该内容

45 篇文章

订阅专栏

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 abstraction – and 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).

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);
}