6

In application which has about 1.5 mln lines of code there is using many pointers as a class members and in other places in code. Classes are usually very huge. It is possible to change to make it safer in quite fast way? Refactor it can take years. It will be hard to change all methods to check if pointer is not null and change pointers to reference where is it can be a reference. It is very important to keep the same performance. I am looking for idea how to refactor it in quite simple and fast way to improve stability of application.

6
  • 3
    Do you have smoke, unit, integration, or performance tests? – bishop Mar 18 '17 at 16:29
  • Performance and integration tests are done by daily basis. There is some of lack of unit tests, about 47% coverage of code. – marcin Mar 18 '17 at 20:48
  • Are you using valgrind at least in some tests? – Basile Starynkevitch Mar 19 '17 at 17:38
  • Roughly, how many developers are actively working on the C++ code of that application? – Doc Brown Mar 20 '17 at 13:22
  • 4
    Well, that makes it a team's issue - you need to train all those people on a regular basis how to write more robust C++ code, how to refactor safely, how to write better tests, and so on. This - and only this - will help you to make your application more stable in the long run, otherwise in the time you fix one issue the 49 other developers add a new one. – Doc Brown Mar 24 '17 at 6:37
11

Let's be very honest: the number of lines of codes that you have will make such a refactoring anything else than easy !

The problem with bad pointers are multiple. For example:

  1. Abusive casting from a pointer of one type to a pointer of an incompatible type (including between types that have different alignment constraints
  2. Dereferencing a nullptr
  3. Dereferencing a pointer to an object that was freed in the meantime

The answer to the first point is to get rid of the c like casting and only use the stricter c++ casts (static_cast, dynamic_cast, and in last ressort with extreme care the reinterpret_cast which highlight potential risks). This can be done without disruption.

Then a lot of nasty pointer issues come with the non respect of the rule of 3 (or rule of 5 nowadays). Go through all your classes that have pointer members and make sure that the rule is enforced. This can be done without disruption.

Similar issues could occur in absence of virtual destructors in polymorphic types. Make sure that all the classes having a virtual function either have a virtual destructor or inherit from a class having its virtual destructor. Still without any disruption.

After these first cleaning steps you have the choice between two other strategies. These are more disruptive and require a heavier investment.

The first one is to opt for smart pointers, with unique_ptr for allocating an object which is owned (and no other than owner can delete this object), shared_ptr when several objects point to the same shared object, (which may be deleted only when no pointers point to it anymore), and weak_ptr which allow to share a pointer but without ownership (e.g. backpointers to parents in a tree). The overhead, thanks to compile time template instantiation and the optimiser, make the overhead minimal if any. This refactoring approach avoids the need to manually delete objects and make the code more reliable (fighting problem 2 and 3). It requires however a good understanding of how each pointer is used. In large code bases, a good option is to deploy this approach by pointed type.

Another more ambitious refactoring approach is to replace pointers and dynamic allocation as much as possible with standard containers (e.g use of string instead of char*, or use of vectors instead of allocating arrays). These containers take care of allocation/deallocation. The c++ move semantic reduces the need to copy objects in many cases. This allows to achieve similar performance as with pointers, but leaving the responsibility for the detailed implementation to the compiler (much more reliably than a human). I think that this would be the preferred refactoring approach but it requires a significant review of all the data structures, pointer usage, and parameter passing).

Both approaches are not mutually exclusive. In some cases you'd still need smart pointers (e.g to implement your own tree structure if a map can't do). In some cases you could opt for smart pointers instead of going for standard containers, especially if the pointers are not well encapsulated and used broadly accross the code.

Conversely, you could opt for smart pointer approach and spot some opportunities for container usage).

Personally, with millions of lines of code, after the first cleaning, if debugging with asserts is not sufficient and if defensive coding could really create a proven and significant overhead, I would opt for this last mixed approach.

1
  • 1
    Classes without virtual functions are also subject to destructor slicing (wrong destructor is invoked on deletion of a pointer to base class). – Basilevs Mar 18 '17 at 20:10
7

Unless you have a very comprehensive suite of automated tests covering all of the code you need to refactor, the odds of this effort passing without any issues is close to zero. There's really no way to make this either simple or fast.

I'd recommend several things:

  1. Get to grips with the root cause - i.e. how on earth did anybody ever let the code get that bad?
  2. Understand how big the task is - how many 'Man years' might this take?
  3. Treat the remedial work like you would treat any new development work.
  4. Make sure everybody in your organisation is heading in the same way and that you're not battling against the tide.

Looking at root causes

  • Why is the code so bad in the first place?
  • What kind of mismanagement caused your code base to slip so deeply into entropy that you are now saddled with so much technical debt?
  • Are the company's current processes (code reviews, developer training, etc.) sufficient to prevent the code base getting worse
  • Do your co-workers really care, or are they content just adding more bad code into the mix?

Bottom Line This effort is pointless if you don't address the underlying causes; before you can really fix problems with entropy and technical debt, you need to put a stop to "technical borrowing", otherwise this will never end and you'll be fighting a losing battle.

Understanding the scale of the problem

You are probably going to be asked a very reasonable question by senior management: "So, exactly what are we getting in return for paying our development team to do this and how much will it cost?"

Even if you don't need to answer that question to your boss, you need to be able to define boundaries, otherwise the task starts to look like a never-ending project.

  • Use a static code analysis tool such as LINT to evaluate the entire code base and let it generate reports for all the problems which you're interested in (e.g. possible memory leaks, possible dangling pointers, possible null dereferencing, etc.)

  • Use your own knowledge of the code base to identify the problematic areas and concentrate on those which suffer from the most severe entropy

  • Keep an eye out for ancient code which might have quick wins - for example, throwing away char* in favour of std::string/iterators, or anywhere in the code which uses anything from an old C header (e.g. scanf, sprintf, malloc, etc.)

  • If you have automated tests covering the areas you find, great. If you do not have automated tests, look carefully at the code to identify the edge cases which should cause the system to misbehave

  • When you find an edge-case which the system cannot handle, define a test (ideally automated) which can prove the existence of defects in that code.

  • Document all of the specific problems you have found, including how to make them fail and why the code needs refactoring; and use this to justify why it should receive attention.

Deciding whether to fix the problems

With such a large code base, you cannot solve everything; your bosses are likely to get nervous if too much money is spent on an effort which they probably already think is going to be like throwing money into a pit.

Do a risk assessment for each area of code which you've identified as being problematic:

  • What happens if something in there breaks?
  • Which customers are affected? How disruptive is it to them?
  • How much money is the company currently spending maintaining that area of code?
  • If the refactoring fails and causes more problems, what's the worst-case scenario? e.g. does it make the whole system unusable for everybody or does it only affect a minor feature rarely used by customers?

Deciding on the best remedy

Start a dialogue with your co-workers (Not just other developers, but anyone who has a stake in the system - especially the product owner) about the problems you've identified. Decide which ones are severe enough to warrant time spent refactoring.

  • Look at the requirement for the code where the problem exists; do any of your customers even use that part of the system any more? can that code just be deleted?

  • Does the problem code generally fulfil the requirements it is intended for or does its design (or lack thereof) mean that there are some weird limitations and caveats? Is it really just a dirty hack? If so, should it be rewritten?

  • Do any of the problem areas overlap with upcoming development work in the pipeline for your team? Consider whether the refactoring effort should be managed as part of a near-future project where the code will need refactoring anyway.

  • Is the overall design of the code just reinventing the wheel from the standard library (or Boost/POCO/etc.)? If so, is it feasible to rip that out and replace it?

  • Is the sitting right in the core of the whole system and too risky to touch? If so, can you refactor pieces around the edges to reduce the coupling?

Getting everybody on board

Efforts like this cannot (or at least, should not) be done by a lone wolf developer. You need the buy-in from everybody in your organisation who has a stake in this.

If it hasn't already happened, the company culture needs to be in a place where both developers and upper management understand the need for taking extra time to improve the quality of code.

Your bosses need to insist that bad code is unacceptable and that proper code reviews are always conducted. Also, experienced developers in the team should be proactive in supporting the rest to raise standards of code being produced.

This is perhaps the hardest problem to solve, but your refactoring effort will fail if you can't get your co-workers on board.

Getting the work underway

I'd strongly recommend doing things slowly; small incremental improvements are significantly less destabilising and easier to test than making huge far-reaching changes across the whole code base.

Don't think of software analogies such as 'skyscraper construction' - that analogy, while commonly used, is heavily flawed because a piece of software is never really finished, and software architecture never fits neatly into an up-front vision or blueprint.

Software is always changing and evolving gradually over time; refactoring really shouldn't be any different.

2

I am looking for idea how to refactor it in quite simple and fast way to improve stability of application.

Personally I would refactor away from the legacy code. Seek to design nice, safe interfaces which communicate with the old code and write automated tests for those new interfaces to ensure their integrity and assumptions. The interfaces can be implemented using the old code for now, but eventually they might stop needing the old code. Don't let the old code heavily influence the shape and design of these interfaces. Focus on the original software features at the user-end level and requirements and design fresh interfaces against those requirements. Take your time designing and testing these interfaces, develop some sense of confidence and pride in the design, but prioritize designing interfaces required for the features your team anticipates will need the most work or for the features your team is planning to add.

Trying to refactor millions of lines of old code is with the mindset of wanting to keep changing it and swimming inside of it against changing requirements in the future. After all, that's the only reason to seek to improve the engineering quality of existing code -- to seek to make it easier to change further down the line. In my opinion that isn't a desirable or very practical goal if there's a sea of code that drives you paranoid. I would seek to reduce the reasons for the old code to ever change in the future, and seek to lock it down using the above technique and possibly even weed it out. Basically start trying to make it work its way towards being increasingly like a third party library in the future, to be called and used if necessary, not modified. And, like any old third party library, if your code and your users becomes less dependent on it in favor of a new library and new features, you might eventually find fewer and fewer reasons to need the "third party library" as it works its way towards obsolescence until the dependencies disappear and you can remove it.

Stop Changing the Old

As you design those new interfaces and tests, even if they use the old nasty code for their implementation, what they do can be reliable and safe and robust as ensured by all the tests you write against these new, well-designed, well-documented, well-tested, safe interfaces that leave little room for common misuse. And that can give your team a big confidence boost as they start writing new code against these new interfaces and not the old, and the dependencies to the old code gradually begin to shrink, like a breath of fresh air after being drowned in a cloud of farts.

Refactoring is seeking to prepare code to better handle further changes. I'd seek the opposite mindset. Seek to increasingly minimize the amount of changes you ever need to make to the old code by seeking to make it stable (as in, lacking reasons for changes, bolted down, cemented in concrete), and seek to make the new code you introduce using the new interfaces you design unstable (the parts that will be fluid and changing against changing requirements). Old code should become increasingly stable and new code should become increasingly unstable. A lot of teams struggling with legacy codebases have this reversed -- their old code is the most unstable while the new code is the most stable, when it should be the opposite.

Code Should Become Stable With Age

In fact, I believe this even for old code which has top-notch engineering. In that case, if it's old, it should have passed the test of time already, proven to be extremely reliable, and should work its way towards a black box library of functionality you can just count on again and again without constantly finding reasons to tamper with it further. Even if you absolutely love working with this old code, there shouldn't be reasons for it to keep changing and changing. You should be able to lock it down with tests and stable interfaces with implementations that work beautifully for years to come instead of having it constantly consisting of moving parts. If you designed proper interfaces, not monoliths that aim to do everything imaginable in the entire world, with goals that can be achieved in a finite amount of time, then there's no reason for any class/library/package to need to be changed and changed indefinitely.

If it has horrid engineering, then there's even more reason to seek to make it stable since your engineers would hate to work in it on top of it being old. So seek stability for things that are starting to age. Seek to make age and stability proportionally related ideas. You should be spending more time working on changing and maintaining new code.

0

The other answers here are really great. I'd like to expand on something mentioned in a few of the other answers. Namely using your tooling to help you get a grip on the problem.

Compiler Warnings

Get your compiler warnings under control. Ideally you should have no warnings when building your application. If you don't already have that, then make a concerted effort to address every single warning your compiler is telling you about. I've seen this done with a very large code base with thousands of warnings. It's possible to get under control, even if it seems helpless. Once you are compiling with no warnings, turn on the compiler setting that sets warnings to errors. This will keep you from ever getting anymore compiler warnings committed to your code base because the app just won't build if there are any warnings. And once you've done that, increase the warning level. Each time you increase the warning level, you fix a whole host of bugs just to get your application building again. Once you've done it once, do it again - turn up the warning level and start fixing.

Static Analysis

It was mentioned to use a linter or static analyzer. (Examples include clang, PVS-Studio, etc.) I could not agree more. Once you've cleared out compiler warnings, you'll still have a bunch of non-obvious issues. Your code will be semantically correct, but might not be doing what you think. A linter or static analyzer will point out things like, "when this value is less than x, y is null, and this later test fails, and memory is leaked." Fixing these sorts of issues gets you even further than fixing the compiler warnings.

Leak Detection & Memory Debugging

Additionally, you should use some sort of leak detection and memory debugging tools such as Instruments, Valgrind, mallocdebug, zombies, or whatever's appropriate for your system. They will tell you when you have allocated memory and not freed it, but no other objects are pointing to it. They will tell you when you are trying to access memory that has already been freed. They will tell you when you are accessing memory that was never allocated. They can find issues with writing past the end of an array or other buffer.

Other Tools

There are a host of other tools for every OS that can point out issues either in your code, or while running. Figure out what the biggest issues are for your users, then find a tool that helps analyze that issue and use it.

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

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