write(this); //not that

One of the first things you learn when you’re starting to develop software is flow control.  By flow control, I mean if-statements, loops and that sort of thing.  Imagine my surprise and subsequent urge to empty the contents of my stomach all over my keyboard when I ran across this today:

private void SetQuantityOnHand(Item forItem)
{

  while (forItem.IsAutoReplenished)
    forItem.QuantityOnHand = 0;

  // Remainder of method removed for brevity.
}

When posting source code to this site I usually make an attempt to justify what the developer was thinking when he wrote the code in question, but this one is pretty hard to justify.  Listen folks, while loops are awesome… when you have something to loop through!  Although I feel that the solution to this one is pretty self-explanatory (or at least I hope that it is), here is how I wound up refactoring the code:

private void SetQuantityOnHand(Item forItem)
{  

  if (forItem.IsAutoReplenished)
    forItem.QuantityOnHand = 0;

  // Remainder of method removed for brevity.
}

That was painless enough.  I usually conclude my posts with a helpful tip but, if you don’t know the difference between an if-statement and a loop, it might be time to hit the books again.

… from what I’ve been seeing lately, this horse isn’t quite dead yet.  As a rule of thumb, if your method is so long that you have to scroll through to take in the whole thing, then you might need to consider refactoring.  Long methods make your code more difficult to read and consequently, more difficult to maintain.  You wouldn’t use a run-on sentence, would you?  Consider the following method from an application I worked on a few years ago:

        /// <summary>
        /// Validate a new request.
        /// the request to validate
        /// Current options for processing the request
        /// thrown if the request does not pass validation
        internal void ValidateNewRequest(EntitlementAccountRequest request)
        {
            // An admin account must have an owner
            if (!request.Account.AccountTypeReference.IsLoaded)
                request.Account.AccountTypeReference.Load();

            if (request.Account.AccountType.AccountTypeID == Enums.AccountType.AdminAccount)
            {
                Check.Require(
                    (DataHelper.GetReferencedEntity(request.Account.OwnerReference) != null),
                    "Admin Account must have an Owner");
            }

            // IF request bypasses approvals AND CurrentPerson is NOT Admin*...
            // * The check for Admin has been specifically implemented here
            // to handle the MassRequest Manual Import functionality.
            // This is the only place where the Entitlement's IsAllowBypassApprovals
            // property is not checked AND the User may bypass approvals for accounts they own.
            if (request.IsBypassApprovals &&
                !RodanRoles.IsCurrentPersonAdmin())
            {
                if (request.Account.Owner != null)
                    Check.Require(request.Requester.PersonID != request.Account.Owner.PersonID,
                                  "Users may not bypass approvals for accounts they own.");
            }

            // validate account does not already have a open request for the entitlement.
            List openRequests =
                GetPendingByAccountID(request.Account.AccountID, true, EntitlementAccountRequest.ENTITLEMENT);
            foreach (EntitlementAccountRequest openRequest in openRequests)
            {
                if (openRequest.Entitlement == request.Entitlement)
                {
                    Check.Assert(false,
                        string.Format("Open request exists for {0}.",
                            openRequest.Account.InformalDisplayDescription));
                }
            }

            // validate account does not already have a map to this request IF we are granting
            if (request.RequestType.RequestTypeID == Enums.RequestType.Grant)
            {
                List existingMaps =
                    BllFactory.EntitlementAccountMapBll.GetCurrentByAccountIDandEntitlementID(
                        request.Account.AccountID,
                        request.Entitlement.EntitlementID
                        );
                Check.Assert(
                    (existingMaps.Count == 0),
                    "Account already has a current (granted) request for this entitlement."
                    );
            }

            if (!request.Entitlement.ApprovalTypeEntitledPartyMaps.IsLoaded)
                request.Entitlement.ApprovalTypeEntitledPartyMaps.Load();
            // ensure that a supervisor exists if one will be required to provide approval.
            // this applies only to SecurePerson recipients who are not users.
            DataHelper.GetReferencedEntity(request.Account.OwnerReference);
            SecurePerson recipientOwner = request.Account.Owner;

            if (!request.IsBypassApprovals
                && (recipientOwner != null)
                && (!(recipientOwner.IsOfficer
                    || (recipientOwner.ArchonHRID == null))
                ))
            {
                foreach (EntitlementApprovalEntitledPartyMap map in request.Entitlement.ApprovalTypeEntitledPartyMaps)
                {
                    EntitledPartyType partyType = DataHelper.GetReferencedEntity(map.EntitledPartyTypeReference);
                    EntitlementApprovalType approvalType =
                        DataHelper.GetReferencedEntity(map.EntitlementApprovalTypeReference);
                    if ((partyType.EntitledPartyTypeID == Enums.EntitledPartyType.Account) &&
                        (approvalType.EntitlementApprovalTypeID == Enums.EntitlementApprovalType.Supervisor))
                    {
                        Check.Require(DataHelper.GetReferencedEntity(recipientOwner.SupervisorReference) != null,
                            "Account Owner must have a supervisor for non-Direct Execute requests.");
                    }
                }
            }
        }

So, granted, the following method is pretty easy to understand with all of the comments, but it does sort of ramble.  I mean, it seems like we’re doing a lot of work for one method.  It might make more sense to split out the functionality into their own seperate methods.  Let’s take a look and see what we can do.

        /// <summary>
        /// Validate a new request.
        /// the request to validate
        /// Current options for processing the request
        /// thrown if the request does not pass validation
        internal void ValidateNewRequest(EntitlementAccountRequest request)
        {
            // 1. An admin account must have an owner
            VerifyOwnership(request); 

            // 2. IF request bypasses approvals AND CurrentPerson is NOT Admin*...
            // * The check for Admin has been specifically implemented here
            // to handle the MassRequest Manual Import functionality.
            // This is the only place where the Entitlement's IsAllowBypassApprovals
            // property is not checked AND the User may bypass approvals for accounts they own.
            VerifyThatApprovalBypassIsAllowed(request);

            // 3. validate account does not already have a open request for the entitlement.
            VerifyThatOpenRequestsDoNotExist(request);

            // 4. validate account does not already have a map to this request IF we are granting
            VerifyNotAlreadyGranted(request);

            // 5. ensure that a supervisor exists if one will be required to provide approval.
            // this applies only to SecurePerson recipients who are not users.
            VerifyThatSupervisorExists(request);
        }

Although there are still a few too many comments for my personal taste, I think that this method is much easier to read and fully comprehend.  It’s also easier to maintain for the next developer that has to modify your code.  Like I said before, if you have to scroll to read the entire method, consider breaking it out.  There are a few cases that I’ve seen where that’s more difficult to do, like an exceptionally long switch statement, but, even still, you may want to reconsider refactoring.

You see it everyday.  Ugly code.  It’s the code that keeps you awake at night.  It’s that 700-line-long method that makes you vomit in your mouth a little bit every time you think about it.  It’s the code that technically works as designed but should, in a perfect world, cost someone their job.  They don’t teach you about this code in a classroom and you can’t learn about it from any book.  You learn about it from experience.  Once you learn to spot it, all that you want to do is fix it.

The purpose of this site is to build a dictionary of good coding practices by examining and analyzing the ugly ones.  By sharing these experiences with the community, you’re helping someone else, be they a junior developer right out of college or a seasoned professional, build better software.  This site is dedicated to just that — building better software.

In order to fully illustrate my point, let’s take a look at an example in C#.  I actually ran across this little gem a few days ago.

try
{
   // Do something important.
}
catch
{
   throw;
}
finally
{
   // Clean-up something important.
}

Of course, I’ve removed some of the meat from this code block, but you get the picture.  We’re doing something important and, afterward, we need to guarantee that some important clean up works gets done.  But, what’s up with the catch block?  It sticks out like a sore thumb.  All it’s doing is re-throwing the exception.  It’s a common misconception that a catch block is always required.  In C#, you can have a catch block, a finally block or both.  So, let’s improve it.

try
{
   // Do something important.
}
finally
{
   // Clean-up something important.
}

Ah, that’s much better.  Of course, if we had some work we actually needed to do in the catch block, we could have left it there, but, for our needs, this is much better.  Someone else may run across your code later and tell themselves “I didn’t know that you could do that.”  As a matter of fact, that’s how I learned most of what I know about building software.  That’s the point of this site.  My goal is to bring that internal dialogue to a global scale and to teach the things that it takes most developers several years out in the real world to really grasp.  There are some things that you can only learn from talking to the guys that have been there and being exposed to the good, the bad and the ugly.

I realize that there are a ton of sites out there dedicated to helping developers build better software, but this site is the site that I’ve always wanted.  I’ve often posted ugly source code to my personal blog or showed it to someone else in the hopes that it could help to teach someone else.  That’s what I want to create here.  The more knowledge we can share, the better a resource we can be.

Jump in.  Help us get this project started.  If you like the it, please don’t keep it a secret.  It doesn’t matter if you’re into C#, Ruby, Perl or anything else.  It doesn’t matter if you’ve been building enterprise applications for years or if you’re just now starting to get your feet wet.  We want to hear from you.

Please keep in mind that this site is very, very beta.  I’m sure that you’ll run across some bugs and, when you do, please let me know about them.  This site is still evolving but, especially at this point, that’s a really good thing.  I want you guys to help mold this site into what it should be.

If you have any questions, please check here first, or feel free to drop me a line.

Welcome.  I look forward to building this with you guys.

Casey.