I have recently "inherited" a project written in ASP.Net MVC which has quite a few issues and I'm looking for suggestions on how to go about refactoring the code.

The technical issues found so far are:

  1. All the business logic is written into stored procedures (1000+ SP's).
  2. No unit tests.
  3. View Models are poorly used.
  4. Views are directly dependent upon the database structures.
  5. JQuery is poorly used - No js frameworks (knockoutjs/angularjs/etc..)
  6. Quality is poor.
  7. Performance is poor.
  8. Non-Responsive Design (though it's meant to work on all devices)
  9. No IOC

The other issues:

  • The application is live and multiple users are using it.
  • New features are being added to it as we speak and approx 40% of features still need to be implemented.
  • The time to refactor is quite limited as the push is on feature release. Rewriting is not an option.
  • Development team would need to be convinced about the shortcomings.

I would like to start small and make incremental improvements, any suggestions will be most welcome.

  • My thoughts on these are: 1. Create system/integration tests (coded ui tests) for the major features of the system.This is to provide a safety net while we go about our refactoring. 2. Run the integration tests every two/three hours to ensure no breaking changes have been checked in 3. Create gated checkins to ensure no breaking code is checked in. 4. Run code analysis (fxcop,stylecop, jshint) as a part of a check-in policy. 5.Create a wrapper over the current database/SP layer and consume the db methods via an interface in the client code.We will then replace the SP's with EF where possibl – vsfm Jan 3 '15 at 15:55
  • 1
    Sounds like you have a fair idea how to proceed. What is your specific question? – Robert Harvey Jan 3 '15 at 16:27
  • My question is what other steps should I take to improve the project (apart from a few that I have posted). My aim is to learn from the practical experience of the developer community to improve a fairly difficult project. – vsfm Jan 3 '15 at 16:37
  • Do you have authoritative control over the project? If you do, you don't need to convince. If you don't, this may all be a bit hopeless. Does the application actually work? – Robert Harvey Jan 3 '15 at 16:38
  • 1
    possible duplicate of I've inherited 200K lines of spaghetti code -- what now? – user53019 Jan 3 '15 at 19:32

Maybe this is somehow useful to you, I will present one way I work on legacy code. There are many more like extracting fields first. It depends, it is different for each piece of legacy code. If it doesn't work the first time, go back one hour (revert changes), try it a second and even a third time, you will get experience at that, under which conditions to start that way or another.

One working way is:

  1. At the first step you should categorize the methods you have, e.g. business, controller, data, ui and sort them within the class to group them. If a method can not be categorized properly, it does to much: Extract smaller methods. categorize these methods too.
  2. At the second step I try to reduce dependencies from the class, where I want to move code away from. You categorized those offending methods, and all these methods should in best case have no dependencies to the class fields or members. This can be done by introducing parameters instead of using class fields into method-signatures. The resulting new method can be potentially moved away.
  3. Third step is to create a new class where one of the categories belong to (in java I'm using inner classes at first and then extract the full class into a separate file, when all depencies are using a public contract) and to create an instance of the new class in the class to clean up.
  4. Now move the easiest method (lowest hanging fruit) into the new class and replace the invocation with an invocation on the new created object.
  5. All these refactorings are meant to cause no differences in runtime execution order. Only apply those refactorings. Try to rely on IDE-Support and double check any change, since you do not have unit-tests yet. When you do small step refactorings, you can stop whenever you need/have to - and deliver the code as is.
  6. Commit often. I mean all one or two minutes. Keep all changes as little as possible and do not try to be clever - no parallel refactorings - one after the other - write a list of steps down - do not try to remember all of them - all it needs to disturb you is a phone call - and your efford is wasted. Do not refactor 10 minutes without commiting. These refactorings are error-prone. Committing often enables you to find the cause of a refactoring error e.g. via binary search on commits.
  7. Now create unit tests for the one single extracted and moved method.
  8. Now move the next low hanging fruit method to new class, write tests etc.
  9. if you have dependencies to the previous class, make these dependencies temporarely public, annotate/document them, that this is not the public API, but needeed for refactoring.
  10. now check, if you need interfaces and extract them to reduce the depencencies between the former class and the new extracted class.

This will step by step remove the code safely from the class. But do not move invocations around within method code, only work with/on full methods. (Optimizations will come next when you have more test coverage)

Now you have unit tests for the new extracted methods and the former class code code is less cluttered. Now you will experience more methods which may belong to the extracted class also. Since you see, who is now invoking the new extracted class. Write more tests.

Proceed with other categories. If you get stuck - it is fine, return another time or sleep a night over it. This will improve the code step by step and no step is expensive at all - only one to two minutes. When you get more experience on this, improve the higher hanging fruits. Start with the lowest hanging ones. Before you add a new feature, try to declutter the code, try to identify the code you need to change and separate it from the code you do not need to touch. Then add new methods using a seam and write test for them.

If you might want a suggestion on a book to read - on legacy code: "Working Effectively with legacy code" from Michael C. Feathers. Also do not miss the great youtube video "Practical Refactoring" on refactoring in very small steps. It is somehow eye opening. But you do not have to agree to them fully.

TL;DR: Well after reading this again this is working on SRP (Single Responsibility Principle) first.

hth a bit.

  • Thanks a ton for the detailed steps and a well thought out answer (especially for the links). This is a very practical, less expensive and less stressful strategy at refactoring to a cleaner code base. My issue is a bit more murkier (as our business logic is in stored procedures) but your advice gives me food for further thoughts. – vsfm Jan 3 '15 at 19:39
  • Maybe someone, can give you his insights too. I would love to read more answers on your question. – thepacker Jan 3 '15 at 20:03

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