7

You've all heard it before, classic Delphi application.

With 3rd party libraries we're at 1.5 million lines of code, probably 200,000 of our own (Dev Express, NexusDB, etc etc)

One huge Datamodule that I've been slowly splitting into 5 (and likely need more). Moving some business logic to these datamodules slowly but surely as methods of the module.

Everything coded 'under the button', no classes of our own. A few forms have 20k lines of code.

I need a reasonable plan to get this in a better place. Right now you can't really test ANY of it, minor changes could introduce an army of bugs, etc.

I was thinking, to start, get a unit going for each major form and extract business logic in the form to this class/unit. Something like TMyForm has a TMyFormClass.pas so that TmyForm ends up with nothing but UI. Continue to modularize the datamodules, write tests asap. Only refactor where we're working on.

Sound sane, addition suggestions, somebody please send me liqour....

Richard Holland
  • 2,663
  • 2
  • 21
  • 35
  • 10
    Buy liquor, drink liquor, forget about code – David Heffernan Dec 19 '11 at 17:06
  • 4
    More seriously, what are your goals? What's driven you to want to change course after 17 years? What benefits are you hoping to gain from refactoring? Answering those questions should guide you in overall strategy. Try to get best bang for your buck. Only refactor for tangible gains. – David Heffernan Dec 19 '11 at 17:13
  • 2
    Be more commercial about it: Ask how much liquor your boss/client is willing to buy to clean it up ? :-) – Marco van de Voort Dec 19 '11 at 17:13
  • @DavidHeffernan The goal is to get testing involved, increase maintainability. Adding features introduces bugs due to all the coupling, lack of testing, etc. I wasn't involved all 17 years and want to make strides to improve the code instead of re-writing. – Richard Holland Dec 19 '11 at 17:17
  • 1
    The most important thing is to get as much code as possible away from anything with a GUI. Then you can test it. But I wouldn't dream of giving anyone advice on the actual mechanics of writing testable code, that's a tricky topic about which I know nothing. I could help with the liquor though! ;-) – David Heffernan Dec 19 '11 at 17:19
  • 1
    I was involved in some refactoring which was required because old software didn't meet numerical standards set down by an external audit (safety related software) - cyclic complexity etc. It was readable but long. End result were a bunch of methods with lots of parameters, neither of which improved readability one iota, in fact made it a whole lot worse. But it met the numerical requirement and that's all that mattered. Apparently. Moral of the story: if it ain't broke... 2nd moral: Fight standards if they don't improve things. Anyway, refactor if you need to, not because you want to. – GeoffM Dec 19 '11 at 17:28
  • 1
    I have been there. First recommendation, stay off the liquor - it will not improve things. Okey 1) the business model is most important. Structure it well and see to it that data flow is stringent. 2) Don't start writing test for GUI until all other work is done. 3) Test the business model without the GUI and divided into logical steps. – LU RD Dec 19 '11 at 17:53
  • With a codebase that old and you will run into code about every other line where you think "why the h. are they doing this? And why here?" Don't take it out because you don't understand why it is there. It probably fixes a bug or works around a corner case either in your own code or in some library used. – Marjan Venema Dec 19 '11 at 18:02
  • 1
    Recommended reading: Working Effectively with Legacy Code (Robert C. Martin) – mjn Dec 19 '11 at 18:46
  • possible duplicate of [How to increase testability?](http://stackoverflow.com/q/7745463/33732) – Rob Kennedy Dec 19 '11 at 18:57
  • 3
    Hopefully you've got a good business case for doing this? If not, you could be heading for trouble. If the business needs increased functionality, improved reliability, and are willing to pay for it (including the required resources and time for regression testing), then you should be ok. If, however, this is your own pet project because you're bored and want to improve things for the sake of quality, or professional pride, etc., you may be building your own coffin. – Chris Thornton Dec 19 '11 at 19:27
  • It's getting harder and harder to implement a customer's requested feature without regression issues. Increased functionality is a need, decreasing bugs added with the feature is a need. – Richard Holland Dec 19 '11 at 19:46
  • I heartily secondn teh "Working Effectively with Legacy Code" recommendation. Written exactly to answer this issue. – Nick Hodges Dec 19 '11 at 22:40
  • I know of someone who has migrated a huge Resource Allocation and Tracking application from the Delphi idiomatic way (DataModule, Dataset, Data aware component, Logic hardcoded in event handlers...) in reasonable delay. Prior to that, he rolled his own application framework and an OPF. – menjaraz Dec 20 '11 at 12:39
  • "Working Effectively with Legacy Code" magically beaming to my Kindle – Richard Holland Dec 20 '11 at 14:46
  • What about Gamekat's answer to http://stackoverflow.com/questions/549369/how-if-to-refactor-a-delphi-program-using-only-forms-and-data-modules ? – Richard Holland Dec 20 '11 at 16:39

4 Answers4

5

In the end, this is a business decision, and should be made as such. What would give your business the most value. Identify that, and refactor that area first, where you can demonstrate (prove) that the benefits to the business are high at least, if not the highest possible value.

I believe that any plan that starts with an implementation idea (MVC rules! Split all the things!) without business-value assessment, is going to be a largely wasted effort.

Example: My application prints and helps me mail my invoices to my customers. Most valuable area of the application to refactor is that I've never been able to handle customer priority and make batching work properly. This improvement has a clear business value, and refactoring it will take me (I estimate) 10 days. During this time, I also hope to decouple my database access code, from my business rules, and decouple my business rules from my UI, for just the area of the code that has to do with the Priority and Batching elements of my UI, which means it hits only 10% of my UI code, and this refactoring effort takes me (I estimate) another 10 days. That means a total of 20 days effort, and the code will be able to do something new. I also intend to make some unit tests (my first ever) that will rely on the ability to mock my database layer away.

Warren P
  • 65,725
  • 40
  • 181
  • 316
4

Adding features? Treat the legacy code like a 3rd party library and write a layer on top of it, then keep your code separate.

Fixing or Modifying? Figure out the easiest way to test. A mixture of unit and hand tests, perhaps faking out some service layer.

Clean up the parts of the code you frequently visit. Don't spend lots of time with the parts you never use. Over time an ideal architecture will become clear as you start to evolve towards it. Only plan when you have to.

Garrett Hall
  • 29,524
  • 10
  • 61
  • 76
  • 1
    The net result of the "layer" approach is a persistent uncurable mental disease. Not recommended. When you lose the ability to ever change your existing code, you're heading down a dead-end road. I call it "code lasagna", a relative of "spaghetti code" but with layers instead of insidious stringy-mesh. – Warren P Dec 19 '11 at 23:04
  • I agree, layers add complexity and indirection that are best avoided. However, when dealing with an enormous, fragile base of legacy code, sometimes adding a layer is the most pragmatic decision. You can always reach underneath that layer when your abstractions leak. – Garrett Hall Dec 24 '11 at 15:47
3

I had this once (600kline serverapp built using a designtime package for html requests dropped in my lap), and I started by trying to reduce line count by identifying similar codepieces, and extracting that to simple classes just to bring down the sheer volume. Copy-and-paste reduction so to speak.

Some modules/forms created by later programmers were of the stringlytyped kind. Cleaning that up was also one of the very first steps.

Doing all that also helped me to get to know the application, and start thinking about a new structure. I first did a round restricting access to/eliminating global variables.

The above steps were mostly worked on in parallel.

Next I started with cleaning up some relative minor modules first to get a feel for it, and then worked my way up to the more major ones. The application remained working for most of this time, with only an occasional week that the codebase was not in a workable state.

The main reason for the cleanup was that the app had been using legacy features of the main component package that had been removed from a newer version. .. which in turn had new features that were needed:-)

In general I'm more for the evolutionary approach in such cases. Massive rewrites and rearchitectures are always over time.

Marco van de Voort
  • 25,628
  • 5
  • 56
  • 89
  • 1
    Probably be my answer. Mainly looking to find out some best practices, etc...a business class to go with a form a decent step vs trying to write an ORM and MVP framework (which I doubt I do on this project). – Richard Holland Dec 19 '11 at 18:31
  • 1
    If you start by making something called `FooBusinessClass` from a form called `FooForm`, and in the end, everything is still coupled to everything, you've gained almost nothing. If you decouple half your application from the other half, you've made a huge leap. – Warren P Dec 19 '11 at 18:36
  • The main reason of the evolutionary bit is to avoid radical decisions too early by doing routine work first, and getting into the problem. So low hanging fruit and no big architectural changes. Though when I wrote the answer I didn't know you also worked on it yourself, so I assumed "getting to know it" would be a major part of the problem – Marco van de Voort Dec 19 '11 at 19:20
  • Seems that tools of the trade are key factor, can you share? – menjaraz Dec 20 '11 at 12:27
2

Firstly I'd try to learn more about how the code works.

To do this get yourself a copy of the SmartInspect or CodeSite logging libraries and log the entry and exit of every single method in your application (SmartInspect has a wizard to do this for you). To complete the picture you should add event handlers for every TDataSet.AfterScroll and TDataSet.AfterOpen event in your datamodule and log the entry/exit of those too.

Next either you or one of your best users needs to run the program and try to exercise every single bit of functionality in it. As you do that you'll be generating trace logs that tell you exactly what methods and database components are used by each form and it's components. Do this in a planned and structured way so that your trace logs aren't themselves an unholy mess.

Next attack that monster data module. I'm assuming you're using dataaware components on your forms, if not see this answer of mine.

I agree that you should be using multiple datamodules however rather than creating them from scratch my advice is to create them by copying and then renaming the monster datamodule. Rather than adding components and code to them, you delete components and code once you're sure it isn't required by the forms it services.

The reason I advise doing this is because it makes your new datamodules very easy to compare against each other and the monster datamodule using Beyond Compare or something similar. If something isn't working right in your new datamodule just load it up in BC against the original monster and you'll be able to see easily what property value the IDE swallowed or which crucial line you eliminated accidently.

Community
  • 1
  • 1
LachlanG
  • 4,047
  • 1
  • 23
  • 35
  • BeyondCompare and any other diff tool is going to quickly drown you in noise, once you start refactoring. Finding one line that got "refactored into nowhere" isn't going to get any easier whether you've got a diff tool or not. – Warren P Dec 22 '11 at 00:12
  • We obviously use BC in different ways as I find a diff tool invaluable in circumstances such as these. :-) – LachlanG Dec 22 '11 at 00:29