Archive for September, 2010

In the twelve years I’ve been developing software I’ve seen quite a few methods, classes, subroutines, scripts, among others. A statement that has been true ever since day one of my development years is that I have spent more time reading code than writing it and I’m sure I don’t have to tell you about some of the horrors I’ve seen during this time. Thinking about this, I’ve come to realize that after so many years of hearing and reading about best practices, self-documented code, small classes and methods, single responsibility purpose, and quite some others nifty concepts it’s amazing how you can still find 500 or more lines of code methods, huge classes that have multiple responsibilities, tightly copuled and low cohesive objects and all sort of development nightmares that make the code difficult to read and maintain.

Surely there is not a single developer who has not written a method only to come back to it some time later and not being able to understand what on earth is it doing, or at the very best to have to spend quite some time figuring it out. And of course that is even more true when we stumble upon a piece of code written by someone else.

With legions of excellent developers all over the world, I truly believe that there can never be enough talk about ways of trying to improve the readability and ease of maintainability of code so, in that tone, I would like to post here a very small demonstration of a piece of code we had at our current project that was, well quite bad, and how it became much more readable after doing some very simple refactoring.

private void SetPermissionToAdjustOrCancell(CustodyTicketDto custodyTicketDto)
{
  var userRole = PortalInternalService.GetRolesforUser(UserSettings.Current.UserId, UserSettings.Current.CompanyId);
  bool canCancelAdjustTicket = false;

  foreach (RoleDto role in userRole)
  {
     if (role.RoleName == Roles.FIELD_OPERATOR_RECEIPT && Activity.ToUpper() == “RECEIPT”)
       canCancelAdjustTicket = true;

     if (role.RoleName == Roles.FIELD_OPERATOR_DELIVERY && Activity.ToUpper() == “DELIVERY”)
       canCancelAdjustTicket = true;
  }

  _view.IsUserAllowedToAdjustOrCancel = canCancelAdjustTicket &&
    (
     (custodyTicketDto.IsCancellable.HasValue && custodyTicketDto.IsCancellable.Value) ||
     (custodyTicketDto.IsAdjustable.HasValue && custodyTicketDto.IsAdjustable.Value)
    );
}

Now this method is by far not the worst anybody has ever seen, but it’s bad enough to illustrate what we should be after. As you can see it has quite a few lines of code, there are several things going on at different levels of abstraction and you need a few moments to analyze each conditional within the method to understand what each one does.

So the first thing we had to do was to write a few unit tests just to make sure we were not breaking anything and once that was set we started breaking up this creepy method into something much more manageable and quite more readable. The end result, at least for now, was this:

private void SetPermissionToAdjustOrCancell(CustodyTicketDto custodyTicketDto)
{
   var userRoles = PortalInternalService.GetRolesforUser(UserSettings.Current.UserId, UserSettings.Current.CompanyId);

   _view.IsUserAllowedToAdjustOrCancel = IsUserInRoleThatAllowsCancelOrAdjust(userRoles) && IsTicketAdjustableOrCancellable(custodyTicketDto);
}

private bool IsUserInRoleThatAllowsCancelOrAdjust(IEnumerable userRoles)
{
   bool canCancelAdjustTicket = false;

   foreach (RoleDto role in userRoles)
   {
      if (IsReceiptRoleAndReceiptActivity(role) || IsDeliveryRoleAndDeliveryActivity(role))
         canCancelAdjustTicket = true;
   }
   return canCancelAdjustTicket;
}

private bool IsReceiptRoleAndReceiptActivity(RoleDto role)
{
   return role.RoleName == Roles.FIELD_OPERATOR_RECEIPT && Activity.ToUpper() ==
“RECEIPT”;
}

private bool IsDeliveryRoleAndDeliveryActivity(RoleDto role)
{
   return role.RoleName == Roles.FIELD_OPERATOR_DELIVERY && Activity.ToUpper() ==
“DELIVERY”;
}

private bool IsTicketAdjustableOrCancellable(CustodyTicketDto custodyTicketDto)
{
   return (custodyTicketDto.IsCancellable.HasValue && custodyTicketDto.IsCancellable.Value) ||
(custodyTicketDto.IsAdjustable.HasValue && custodyTicketDto.IsAdjustable.Value);
}

Now I am very well aware that this method could still be improved, for example the loop in the IsUserInRoleThatAllowsCancelOrAdjust method could very well be replaced by linq queries or that the IsTicketAdjustableOrCancellable could be broken in two methods, but the point is that now someone who has never seen this code will have very little trouble if at all to understand what each method is doing, and without having to add a single comment !!!!

I do not mean to say that this is THE way to go but at least it has been working for us. We’ve had a few new developers say that it is so simple to understand this kind of code and add features or fix defects in it so, if you think this kind of style could be beneficial I encourage you to give it a try and decide if it could prove to be usefull in the long run.