Take the 2-minute tour ×
Programmers Stack Exchange is a question and answer site for professional programmers interested in conceptual questions about software development. It's 100% free, no registration required.

I have been reading Working Effectively with Legacy Code and Clean Code with the goal of learning strategies on how to begin cleaning up the existing code-base of a large ASP.NET webforms application.

This system has been around since 2005 and since then has undergone a number of enhancements. Originally the code was structured as follows (and is still largely structured this way):

  • ASP.NET (aspx/ascx)
  • Code-behind (c#)
  • Business Logic Layer (c#)
  • Data Access Layer (c#)
  • Database (Oracle)

The main issue is that the code is procedural masquerading as object-oriented. It virtually violates all of the guidelines described in both books.

This is an example of a typical class in the Business Logic Layer:

    public class AddressBO
    public TransferObject GetAddress(string addressID)
        if (StringUtils.IsNull(addressID))
            throw new ValidationException("Address ID must be entered");

        AddressDAO addressDAO = new AddressDAO();
        return addressDAO.GetAddress(addressID);

    public TransferObject Insert(TransferObject addressDetails)
        if (StringUtils.IsNull(addressDetails.GetString("EVENT_ID")) ||
            StringUtils.IsNull(addressDetails.GetString("LOCALITY")) ||
            StringUtils.IsNull(addressDetails.GetString("ADDRESS_TARGET")) ||
            StringUtils.IsNull(addressDetails.GetString("ADDRESS_TYPE_CODE")) ||
            throw new ValidationException(
                "You must enter an Event ID, Locality, Address Target, Address Type Code and Created By.");

        string addressID = Sequence.GetNextValue("ADDRESS_ID_SEQ");
        addressDetails.SetValue("ADDRESS_ID", addressID);

        string syncID = Sequence.GetNextValue("SYNC_ID_SEQ");
        addressDetails.SetValue("SYNC_ADDRESS_ID", syncID);

        TransferObject syncDetails = new TransferObject();

        Transaction transaction = new Transaction();

            AddressDAO addressDAO = new AddressDAO();
            addressDAO.Insert(addressDetails, transaction);

            // insert the record for the target
            TransferObject addressTargetDetails = new TransferObject();
            switch (addressDetails.GetString("ADDRESS_TARGET"))
                case "PARTY_ADDRESSES":
                        addressTargetDetails.SetValue("ADDRESS_ID", addressID);
                        addressTargetDetails.SetValue("PARTY_ID", addressDetails.GetString("PARTY_ID"));
                        addressTargetDetails.SetValue("EVENT_ID", addressDetails.GetString("EVENT_ID"));
                        addressTargetDetails.SetValue("CREATED_BY", addressDetails.GetString("CREATED_BY"));

                        addressDAO.InsertPartyAddress(addressTargetDetails, transaction);

                case "PARTY_CONTACT_ADDRESSES":
                        addressTargetDetails.SetValue("ADDRESS_ID", addressID);
                        addressTargetDetails.SetValue("CONTACT_ID", addressDetails.GetString("CONTACT_ID"));
                        addressTargetDetails.SetValue("EVENT_ID", addressDetails.GetString("EVENT_ID"));
                        addressTargetDetails.SetValue("CREATED_BY", addressDetails.GetString("CREATED_BY"));

                        addressDAO.InsertContactAddress(addressTargetDetails, transaction);


                << many more cases here >>

            // synchronise
            SynchronisationBO synchronisationBO = new SynchronisationBO();
            syncDetails = synchronisationBO.Synchronise("I", transaction,
                                                        "ADDRESSES", addressDetails.GetString("ADDRESS_TARGET"),
                                                        addressDetails, addressTargetDetails);

            // commit
        catch (Exception)

        return new TransferObject("ADDRESS_ID", addressID, "SYNC_DETAILS", syncDetails);

    << many more methods are here >>


It has a lot of duplication, the class has a number of responsibilities, etc, etc - it is just generally 'un-clean' code.

All of the code throughout the system is dependent on concrete implementations.

This is an example of a typical class in the Data Access Layer:

    public class AddressDAO : GenericDAO
    public static readonly string BASE_SQL_ADDRESSES =
        "SELECT " +
        "  a.address_id, " +
        "  a.event_id, " +
        "  a.flat_unit_type_code, " +
        "  fut.description as flat_unit_description, " +
        "  a.flat_unit_num, " +
        "  a.floor_level_code, " +
        "  fl.description as floor_level_description, " +
        "  a.floor_level_num, " +
        "  a.building_name, " +
        "  a.lot_number, " +
        "  a.street_number, " +
        "  a.street_name, " +
        "  a.street_type_code, " +
        "  st.description as street_type_description, " +
        "  a.street_suffix_code, " +
        "  ss.description as street_suffix_description, " +
        "  a.postal_delivery_type_code, " +
        "  pdt.description as postal_delivery_description, " +
        "  a.postal_delivery_num, " +
        "  a.locality, " +
        "  a.state_code, " +
        "  s.description as state_description, " +
        "  a.postcode, " +
        "  a.country, " +
        "  a.lock_num, " +
        "  a.created_by, " +
        "  TO_CHAR(a.created_datetime, '" + SQL_DATETIME_FORMAT + "') as created_datetime, " +
        "  a.last_updated_by, " +
        "  TO_CHAR(a.last_updated_datetime, '" + SQL_DATETIME_FORMAT + "') as last_updated_datetime, " +
        "  a.sync_address_id, " +
        "  a.lat," +
        "  a.lon, " +
        "  a.validation_confidence, " +
        "  a.validation_quality, " +
        "  a.validation_status " +
        "WHERE a.flat_unit_type_code = fut.flat_unit_type_code(+) " +
        "AND   a.floor_level_code = fl.floor_level_code(+) " +
        "AND   a.street_type_code = st.street_type_code(+) " +
        "AND   a.street_suffix_code = ss.street_suffix_code(+) " +
        "AND   a.postal_delivery_type_code = pdt.postal_delivery_type_code(+) " +
        "AND   a.state_code = s.state_code(+) ";

    public TransferObject GetAddress(string addressID)
        //Build the SELECT Statement
        StringBuilder selectStatement = new StringBuilder(BASE_SQL_ADDRESSES);

        //Add WHERE condition
        selectStatement.Append(" AND a.address_id = :addressID");

        ArrayList parameters = new ArrayList{DBUtils.CreateOracleParameter("addressID", OracleDbType.Decimal, addressID)};

        // Execute the SELECT statement
        Query query = new Query();
        DataSet results = query.Execute(selectStatement.ToString(), parameters);

        // Check if 0 or more than one rows returned
        if (results.Tables[0].Rows.Count == 0)
            throw new NoDataFoundException();
        if (results.Tables[0].Rows.Count > 1)
            throw new TooManyRowsException();

        // Return a TransferObject containing the values
        return new TransferObject(results);

    public void Insert(TransferObject insertValues, Transaction transaction)
        // Store Values
        string addressID = insertValues.GetString("ADDRESS_ID");
        string syncAddressID = insertValues.GetString("SYNC_ADDRESS_ID");
        string eventID = insertValues.GetString("EVENT_ID");
        string createdBy = insertValues.GetString("CREATED_BY");

        // postal delivery
        string postalDeliveryTypeCode = insertValues.GetString("POSTAL_DELIVERY_TYPE_CODE");
        string postalDeliveryNum = insertValues.GetString("POSTAL_DELIVERY_NUM");

        // unit/building
        string flatUnitTypeCode = insertValues.GetString("FLAT_UNIT_TYPE_CODE");
        string flatUnitNum = insertValues.GetString("FLAT_UNIT_NUM");
        string floorLevelCode = insertValues.GetString("FLOOR_LEVEL_CODE");
        string floorLevelNum = insertValues.GetString("FLOOR_LEVEL_NUM");
        string buildingName = insertValues.GetString("BUILDING_NAME");

        // street
        string lotNumber = insertValues.GetString("LOT_NUMBER");
        string streetNumber = insertValues.GetString("STREET_NUMBER");
        string streetName = insertValues.GetString("STREET_NAME");
        string streetTypeCode = insertValues.GetString("STREET_TYPE_CODE");
        string streetSuffixCode = insertValues.GetString("STREET_SUFFIX_CODE");

        // locality/state/postcode/country
        string locality = insertValues.GetString("LOCALITY");
        string stateCode = insertValues.GetString("STATE_CODE");
        string postcode = insertValues.GetString("POSTCODE");
        string country = insertValues.GetString("COUNTRY");

        // esms address
        string esmsAddress = insertValues.GetString("ESMS_ADDRESS");

        string lat = insertValues.GetString("LAT");
        string lon = insertValues.GetString("LON");
        string zoom = insertValues.GetString("ZOOM");

        //string validateDate = insertValues.GetString("VALIDATED_DATE");
        string validatedBy = insertValues.GetString("VALIDATED_BY");
        string confidence = insertValues.GetString("VALIDATION_CONFIDENCE");
        string status = insertValues.GetString("VALIDATION_STATUS");
        string quality = insertValues.GetString("VALIDATION_QUALITY");

        // the insert statement
        StringBuilder insertStatement = new StringBuilder("INSERT INTO ADDRESSES (");
        StringBuilder valuesStatement = new StringBuilder("VALUES (");

        ArrayList parameters = new ArrayList();

        // build the insert statement
        valuesStatement.Append(":addressID, :eventID, :createdBy, SYSDATE, 1 ");
        parameters.Add(DBUtils.CreateOracleParameter("addressID", OracleDbType.Decimal, addressID));
        parameters.Add(DBUtils.CreateOracleParameter("eventID", OracleDbType.Decimal, eventID));
        parameters.Add(DBUtils.CreateOracleParameter("createdBy", OracleDbType.Varchar2, createdBy));

        // build the insert statement
        if (!StringUtils.IsNull(syncAddressID))
            insertStatement.Append(", SYNC_ADDRESS_ID");
            valuesStatement.Append(", :syncAddressID");
            parameters.Add(DBUtils.CreateOracleParameter("syncAddressID", OracleDbType.Decimal, syncAddressID));

        if (!StringUtils.IsNull(postalDeliveryTypeCode))
            insertStatement.Append(", POSTAL_DELIVERY_TYPE_CODE");
            valuesStatement.Append(", :postalDeliveryTypeCode ");
            parameters.Add(DBUtils.CreateOracleParameter("postalDeliveryTypeCode", OracleDbType.Varchar2, postalDeliveryTypeCode));

        if (!StringUtils.IsNull(postalDeliveryNum))
            insertStatement.Append(", POSTAL_DELIVERY_NUM");
            valuesStatement.Append(", :postalDeliveryNum ");
            parameters.Add(DBUtils.CreateOracleParameter("postalDeliveryNum", OracleDbType.Varchar2, postalDeliveryNum));

        if (!StringUtils.IsNull(flatUnitTypeCode))
            insertStatement.Append(", FLAT_UNIT_TYPE_CODE");
            valuesStatement.Append(", :flatUnitTypeCode ");
            parameters.Add(DBUtils.CreateOracleParameter("flatUnitTypeCode", OracleDbType.Varchar2, flatUnitTypeCode));

        if (!StringUtils.IsNull(lat))
            insertStatement.Append(", LAT");
            valuesStatement.Append(", :lat ");
            parameters.Add(DBUtils.CreateOracleParameter("lat", OracleDbType.Decimal, lat));

        if (!StringUtils.IsNull(lon))
            insertStatement.Append(", LON");
            valuesStatement.Append(", :lon ");
            parameters.Add(DBUtils.CreateOracleParameter("lon", OracleDbType.Decimal, lon));

        if (!StringUtils.IsNull(zoom))
            insertStatement.Append(", ZOOM");
            valuesStatement.Append(", :zoom ");
            parameters.Add(DBUtils.CreateOracleParameter("zoom", OracleDbType.Decimal, zoom));

        if (!StringUtils.IsNull(flatUnitNum))
            insertStatement.Append(", FLAT_UNIT_NUM");
            valuesStatement.Append(", :flatUnitNum ");
            parameters.Add(DBUtils.CreateOracleParameter("flatUnitNum", OracleDbType.Varchar2, flatUnitNum));

        if (!StringUtils.IsNull(floorLevelCode))
            insertStatement.Append(", FLOOR_LEVEL_CODE");
            valuesStatement.Append(", :floorLevelCode ");
            parameters.Add(DBUtils.CreateOracleParameter("floorLevelCode", OracleDbType.Varchar2, floorLevelCode));

        if (!StringUtils.IsNull(floorLevelNum))
            insertStatement.Append(", FLOOR_LEVEL_NUM");
            valuesStatement.Append(", :floorLevelNum ");
            parameters.Add(DBUtils.CreateOracleParameter("floorLevelNum", OracleDbType.Varchar2, floorLevelNum));

        if (!StringUtils.IsNull(buildingName))
            insertStatement.Append(", BUILDING_NAME");
            valuesStatement.Append(", :buildingName ");
            parameters.Add(DBUtils.CreateOracleParameter("buildingName", OracleDbType.Varchar2, buildingName));

        if (!StringUtils.IsNull(lotNumber))
            insertStatement.Append(", LOT_NUMBER");
            valuesStatement.Append(", :lotNumber ");
            parameters.Add(DBUtils.CreateOracleParameter("lotNumber", OracleDbType.Varchar2, lotNumber));

        if (!StringUtils.IsNull(streetNumber))
            insertStatement.Append(", STREET_NUMBER");
            valuesStatement.Append(", :streetNumber ");
            parameters.Add(DBUtils.CreateOracleParameter("streetNumber", OracleDbType.Varchar2, streetNumber));

        if (!StringUtils.IsNull(streetName))
            insertStatement.Append(", STREET_NAME");
            valuesStatement.Append(", :streetName ");
            parameters.Add(DBUtils.CreateOracleParameter("streetName", OracleDbType.Varchar2, streetName));

        if (!StringUtils.IsNull(streetTypeCode))
            insertStatement.Append(", STREET_TYPE_CODE");
            valuesStatement.Append(", :streetTypeCode ");
            parameters.Add(DBUtils.CreateOracleParameter("streetTypeCode", OracleDbType.Varchar2, streetTypeCode));

        if (!StringUtils.IsNull(streetSuffixCode))
            insertStatement.Append(", STREET_SUFFIX_CODE");
            valuesStatement.Append(", :streetSuffixCode ");
            parameters.Add(DBUtils.CreateOracleParameter("streetSuffixCode", OracleDbType.Varchar2, streetSuffixCode));

        if (!StringUtils.IsNull(locality))
            insertStatement.Append(", LOCALITY");
            valuesStatement.Append(", :locality");
            parameters.Add(DBUtils.CreateOracleParameter("locality", OracleDbType.Varchar2, locality));

        if (!StringUtils.IsNull(stateCode))
            insertStatement.Append(", STATE_CODE");
            valuesStatement.Append(", :stateCode");
            parameters.Add(DBUtils.CreateOracleParameter("stateCode", OracleDbType.Varchar2, stateCode));

        if (!StringUtils.IsNull(postcode))
            insertStatement.Append(", POSTCODE");
            valuesStatement.Append(", :postcode ");
            parameters.Add(DBUtils.CreateOracleParameter("postcode", OracleDbType.Varchar2, postcode));

        if (!StringUtils.IsNull(country))
            insertStatement.Append(", COUNTRY");
            valuesStatement.Append(", :country ");
            parameters.Add(DBUtils.CreateOracleParameter("country", OracleDbType.Varchar2, country));

        if (!StringUtils.IsNull(esmsAddress))
            insertStatement.Append(", ESMS_ADDRESS");
            valuesStatement.Append(", :esmsAddress ");
            parameters.Add(DBUtils.CreateOracleParameter("esmsAddress", OracleDbType.Varchar2, esmsAddress));

        if (!StringUtils.IsNull(validatedBy))
            insertStatement.Append(", VALIDATED_DATE");
            valuesStatement.Append(", SYSDATE ");
            insertStatement.Append(", VALIDATED_BY");
            valuesStatement.Append(", :validatedBy ");
            parameters.Add(DBUtils.CreateOracleParameter("validatedBy", OracleDbType.Varchar2, validatedBy));

        if (!StringUtils.IsNull(confidence))
            insertStatement.Append(", VALIDATION_CONFIDENCE");
            valuesStatement.Append(", :confidence ");
            parameters.Add(DBUtils.CreateOracleParameter("confidence", OracleDbType.Decimal, confidence));

        if (!StringUtils.IsNull(status))
            insertStatement.Append(", VALIDATION_STATUS");
            valuesStatement.Append(", :status ");
            parameters.Add(DBUtils.CreateOracleParameter("status", OracleDbType.Varchar2, status));

        if (!StringUtils.IsNull(quality))
            insertStatement.Append(", VALIDATION_QUALITY");
            valuesStatement.Append(", :quality ");
            parameters.Add(DBUtils.CreateOracleParameter("quality", OracleDbType.Decimal, quality));

        // finish off the statement
        insertStatement.Append(") ");

        // build the insert statement
        string sql = insertStatement + valuesStatement.ToString();

        // Execute the INSERT Statement
        Dml dmlDAO = new Dml();
        int rowsAffected = dmlDAO.Execute(sql, transaction, parameters);

        if (rowsAffected == 0)
            throw new NoRowsAffectedException();

    << many more methods go here >>

This system was developed by me and a small team back in 2005 after a 1 week .NET course. Before than my experience was in client-server applications. Over the past 5 years I've come to recognise the benefits of automated unit testing, automated integration testing and automated acceptance testing (using Selenium or equivalent) but the current code-base seems impossible to introduce these concepts.

We are now starting to work on a major enhancement project with tight time-frames. The team consists of 5 .NET developers - 2 developers with a few years of .NET experience and 3 others with little or no .NET experience. None of the team (including myself) has experience in using .NET unit testing or mocking frameworks.

What strategy would you use to make this code cleaner, more object-oriented, testable and maintainable?

share|improve this question
As an aside, it may be worth double checking that there's a cost justification to rewriting the system. The old code may be ugly, but if it works well enough it may be cheaper to put with the rough edges and invest your development time elsewhere. –  smithco Feb 21 '11 at 4:03
One possible justification is to reduce the effort and cost of manual re-testing after every enhancement project. At the end of the last project, manual testing went for about 2 months. If the introduction of more automated testing reduces this effort to 1-2 weeks it might be worth it. –  Anthony Feb 21 '11 at 4:16
I agree that is reasonably consistent and structured. My main aim is to reduce side-effects of change. The effort required to manually test the entire application after (and during) each project is massive. I had thought about using Selenium to test it via the client-side - I have a question on ServerFault (serverfault.com/questions/236546/…) to get suggestions on quickly reverting the database. I feel automated acceptance testing would get most of the benefits without having to do a massive rewrite. –  Anthony Feb 21 '11 at 5:24
add comment

6 Answers

up vote 10 down vote accepted

You mention two books in which one of the primary messages is "The Boy Scout Rule" i.e. clean up the code as you touch it. If you have a working system, a massive rewrite is counter-productive. Instead, as you add new functionality, make sure you improve the code as it stands.

  • Write unit tests to cover the existing code that you need to change.
  • Refactor that code so it is more pliable for change (making sure your tests still pass).
  • Write tests for the new/revised functionality
  • Write code to make the new tests pass
  • Refactor as necessary.
share|improve this answer
I agree - I liked this strategy when I read about it. We could spend months cleaning up the code without really adding value. If we focus on cleaning up what we need to change while adding a new feature we get the best of both worlds. –  Anthony Feb 21 '11 at 21:44
The only challenge is writing the unit tests for the existing code. I'm leaning more towards higher-level tests first so that we can refactor and add unit tests with more confidence. –  Anthony Feb 21 '11 at 21:46
Yes the best you can do is write tests that verifies the code does what it does. You can try to create tests that verify correct behavior...but you run the risk of breaking other functionality that isn't covered by tests. –  Mike Brown Feb 22 '11 at 4:12
add comment

For such a legacy app, it is much more cost-effective to start by covering it with (automated) higher level integration tests rather than unit tests. Then with the integration tests as your safety net, you can start refactoring in small steps if it is appropriate, i.e. if the cost of refactoring pays itself back over the long term. As others have noted, this is not self evident.

See also this earlier answer of mine to a similar question; hope you find it useful.

share|improve this answer
I'm leaning towards higher level tests to start with. I'm working with the DBA to work out the best way to revert the database after each test execution. –  Anthony Feb 21 '11 at 21:48
add comment

If I remember right Working Effectively with Legacy Code says a full on rewrite doesn't guarantee that the new code would be any better than the old (from a functionality/defects point of view). The refactorings in that book are for when fixing bugs/adding new features.

Another book I would recommend is Brownfield Application Development in .NET which basically says not to do a full on rewrite as well. It talks about making steady, iterative changes whenever you add new features or fix defects. It addresses the cost vs. benefits considerations and warns about trying to bit off too much at one time. While Working Effectively with Legacy Code talks mostly about how to refactor on the micro/code level, Brownfield Application Development in .NET, mostly covers the higher level considerations when re-factoring (along with some code-level stuff too).

The Brownfield book also suggests figuring out which area of the code is causing you the most trouble and focus there. Any other areas that aren't needing a lot of maintenance may not be worth changing.

share|improve this answer
+1 for the book Brownfield Application Development in .Net –  Gabriel Mongeon Feb 21 '11 at 15:56
Thanks for the book recommendation - I'll take a look at it. From the overview it will be more focussed on .NET specifically than the two books I mentioned which seem to focus on C, C++ and Java. –  Anthony Feb 21 '11 at 21:47
add comment

Be very careful with throwing away and rewritting running code (Things you should never do). Sure it may be ugly, but if it works leave it be. See Joel's blog post, sure its 10+ years old, but its still right on target.

share|improve this answer
I disagree with Joel there. What he said may have felt relevant at the time but isn't the rewrite what is now called Mozilla Firefox? –  CashCow Feb 21 '11 at 9:48
Yes, but it put netscape out of business in the process! Its not saying that starting over is never the right choice but its something to be very careful about. And OO code is not always better then procedural code. –  Zachary K Feb 21 '11 at 10:25
add comment

Those are both good books. If your going to start rewriting code in that manner I think it's important to also begin to cover the code with unit tests to help keep it stable as you rewrite it.

It has to be done in small steps and modifying that kind of code can easily destabilize the entire system.

I would not modify any code you are not actively working on. Only do this on code you are actively enhancing or fixing. If something is serving it's purpose but hasn't been modified in years then just leave it. It's doing it's thing even if you know a better way.

At the end of the day the company needs productivity. While better code increases productivity rewriting code just cause it could be written better is probably not the best way to bring value to your product.

share|improve this answer
add comment

As Mike stated the 'boy scout rule' is probably best here, if the code works and isn't a constant source of bug reports I would prefer to let it sit there and improve it slowly over time.

During your enhancement project allow for new ways of doing things. For instance use an ORM for new features and bypass the existing data layer pattern. When you run into enhancements that need to touch existing code you may be able to move some of the related code to the new way of doing things. Using a facade or some adapters at places can help you isolate the old code, perhaps even per layer. This could help you to starve the old code through time.

Similarly this can help you with adding unit tests, you can start with the new code you make and slowly add some tests for old code you have to touch for the new enhancements.

share|improve this answer
add comment

Your Answer


By posting your answer, you agree to the privacy policy and terms of service.

Not the answer you're looking for? Browse other questions tagged or ask your own question.