ASP.NET Security Consultant

I'm an author, speaker, and generally a the-way-we've-always-done-it-sucks security guy who specializes in web technologies and ASP.NET.

I'm a bit of a health nut too, having lost 50 pounds in 2019 and 2020. Check out my blog for weight loss tips!

The importance of rewriting working code

Published on: 2012-01-02

Refactoring code, or the process of rewriting code that already works, is a vital part of maintaining the long-term health of a software application. Most business stakeholders do not wish to pay for this, though. Most objections I've heard fall under two categories:

  • Rewriting working code will provide no benefits and is a waste of time
  • The need for refactoring is a sign that the developer didn't get it right the first time, and the budget shouldn't have to suffer for a development mistake

Both of these objections are understandable, but I'll explain why they don't apply. Think of refactoring code is more of a software maintenance process; just like getting your oil changed in your car or getting your air ducts cleaned in your house are necessary maintenance. To see why, take a look at this scenario of what could happen if you don’t maintain your code. In this example, I'm designing a page that allows users to enter orders for widgets. Imagine that the system needs to show a particular page to two types of users, Sales and Procurement (who fulfills the order). Sales people will be able to enter as many orders as they wish, but procurement will only be able to see or edit orders that have been finalized by the sales person.

Pseudo-code:

If User is Procurer and Order is Finalized
    Show Page
Else If User is Procurer and Order is Not Finalized
    Hide Page
Else if User is Sales and Order is Finalized
    Hide Page
Else if User is Sales and Order is Not Finalized
    Show Page

This code is already pretty complex and is far from ideal, but it's manageable.

Changes in software are typical. Continuing with this example, let's imagine that this software needed to be changed to make sure that orders are completed on time. Supervisors would need to see the page too, but they can't fulfill orders like Procurement. They can send notes to parties involved and override settings, though. Sales people need to see orders they've sent to Procurement.

Pseudo-code:

If User is Procurer and Order is Finalized
    Show Page
Else If User is Procurer and Order is Not Finalized
    Hide Page
Else if User is Sales and Order is Finalized
    Show Page
    Make page read-only somehow
Else if User is Sales and Order is Not Finalized
    Show Page
Else if User is Supervisor
    Show page
    Hide finalization capability
    Show supervisor-specific controls

Ok, it's still not terrible, but it's getting harder to read and maintain.

For the next change in our example, we'll add the ability for Supervisors to talk to our customers and enter sales in as well. But a Supervisor can't supervise his/her own sale.

Pseudo-code:

If User is Procurer and Order is Finalized
    Show Page
Else If User is Procurer and Order is Not Finalized
    Hide Page
Else if User is Sales and Order is Finalized
    Show Page
    Make page read-only somehow
Else if User is Sales and Order is Not Finalized
    Show Page
Else if User is Supervisor
    Show page
    If Supervisor did the sale and Order is Finalized
        Make page read-only somehow
    Else if Supervisor did the sale and Order is not finalized
        (We already showed the page, so we're good)
    Else if supervisor did not create the sale
        (We already showed the page)
        Show supervisor section
        Hide finalization capability

That's quite a bit worse. You can imagine coming to this code as a programmer new to the project and being confused by this approach. It's also complicated enough where we'd risk breaking something if we make another change. Now, let's refactor:

Get the user that's viewing the page
If user can read page
    Show Page
If user cannot finalize
    Hide finalization
If user cannot edit
    Make page read only somehow
If the user is the order's supervisor
    Show supervisor information

In a separate area of the code, you'll define the method of getting a user like this:

If the user is a supervisor
    Get the supervisor information
If the user is a sales person
    Get the sales person information
If the user is in procurement
    Get the procurer information

Finally, we would define each user separately. Here's the supervisor's pseudo-code as an example:

Can I read the page?
    Yes

Can I edit the page?
    If I'm the salesperson and the order is finalized
        No
    If I'm the salesperson and the order is not finalized
        Yes
    If I'm not the salesperson
        Yes

Can I finalize the page?
    Only if I'm the sales person.

Am I the order's supervisor?
    Only if I'm not the sales person.

This code probably looks more complex than the first example. However, by changing the code in this way, we are hiding the complexity of the code for any given point in time and allowing the developer to only see what's needed at any given moment. Do you need to see a big picture view of what's happening? Just look at the top-level code. Do you need to see what the supervisor can do at a given moment? That's pretty easy to do too. This code is easier to read and edit, making bug fixes and adding new features significantly easier, lowering the costs of maintaining the software significantly. Also keep in mind that this is a relatively simple scenario. Imagine looking at hundreds of thousands of lines of code that looks like the last pre-refactored example. Not pretty! So perhaps the next time you're faced with paying for refactoring code, try to imagine maintaining this piece of software for years to come, and think of the long-term health of your software.

This article was originally posted here and may have been edited for clarity.