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.

Sometimes you run into a situation where you have to extend/improve some existing code. You see that the old code is very lean, but it's also difficult to extend, and takes time to read.

Is it a good idea to replace it with modern code?

Some time ago I liked the lean approach, but now, it seems to me that it's better to sacrifice a lot of optimizations in favor of higher abstractions, better interfaces and more readable, extendable code.

The compilers seem to be getting better as well, so things like struct abc = {} are silently turned into memsets, shared_ptrs are pretty much producing the same code as raw pointer twiddling, templates are working super good because they produce super lean code, and so on.

But still, sometimes you see stack based arrays and old C functions with some obscure logic, and usually they are not on the critical path.

Is it a good idea to change such code if you have to touch a small piece of it either way?

share|improve this question
18  
Readability and optimizations are not opposed most of the time. –  deadalnix Jun 11 '12 at 16:38
21  
Can the readability improve with some comments? –  YetAnotherUser Jun 11 '12 at 18:15
12  
It's worrying that OOP-ification is considered 'modern code' –  James Jun 11 '12 at 19:20
4  
@StevenBurnap: Well, sub esp 0x100 will always beat the heap based std::vector even with reserve, and also, first one is nothrow while the second is free to throw. But these are micro optimizations, and when coupled with pointer arithmetic, IMHO, should be replaced with std::* alternatives in most places. –  Coder Jun 11 '12 at 19:45
7  
like slackware philosophy : if it isn't broken don't fix it, at less you have a very, very good reason to do it –  osdamv Jun 11 '12 at 21:19
show 5 more comments

19 Answers

If I could teach the world one thing (about Software) before I die, I'd teach it that "Performance versus X" is a False Dilemma.

Refactoring is typically known as a boon for readability and reliability, but it can just as easily support optimization. When you handle performance improvement as a series of refactorings, you can honor the Campsite Rule while also making the application go faster. It is actually, at least in my opinion, ethically incumbent upon you to do so.

For instance, the author of this question has encountered a crazy piece of code. If this person were reading my code, they would find that the crazy part is 3-4 lines long. It's in a method by itself, and the method name and description indicates WHAT the method is doing. The method would contain 2-6 lines of inline comments describing HOW the crazy code gets the right answer, despite its questionable appearance.

Compartmentalized in this way, you are free to swap out implementations of this method as you like. Indeed, that's probably how I wrote the crazy version to begin with. You are welcome to try, or at least ask about alternatives. Most of the time you will find out that the naive implementation is noticeably worse (usually I only bother for a 2-10x improvement), but compilers and libraries are always changing, and who knows what you may find today that wasn't available when the function was written?

share|improve this answer
add comment

The answer is, without loss of generality, yes. Always add modern code when you see hard to read code, and delete the bad code in most cases. I use the following process:

  1. Look for the performance test and supporting profiling information. If there is no performance test, then what can be asserted without evidence can be dismissed without evidence. Assert that your modern code is faster and remove the old code. If anyone argues (even yourself) ask them to write the profiling code to prove which is faster.
  2. If the profiling code exists, write the modern code anyway. Name it something like <function>_clean(). Then, "race" your code against the bad code. If your code is better, remove the old code.
  3. If the old code is faster, leave your modern code in there anyway. It serves as good documentation for what the other code is meant to do, and since the "race" code is there, you can keep running it for documenting the performance characteristics and differences between the two paths. You can also unit test for differences in code behaviour. Importantly, the modern code will beat the "optimised" code one day, guaranteed. You can then remove the bad code.

QED.

share|improve this answer
add comment

Here is an example to see the difference between simple code and optimized code: http://stackoverflow.com/a/11227902/1396264

towards the end of the answer he just Replaces:

if (data[c] >= 128)
    sum += data[c];

with:

int t = (data[c] - 128) >> 31;
sum += ~t & data[c];

To be fair I have no idea what the if statement has been replaced with but as the answerer says its some bitwise operations giving the same result (I'm just going to take his word for it).

This executes in less than a quarter of the original time (11.54sec vs 2.5sec)

share|improve this answer
add comment

IMHO readability is more important than optimised code because in most cases micro-optimisation does not worth it.

Article about non-sense micro-optimizations:

As most of us, I am tired to read blog posts about non-sense micro-optimizations like replacing print by echo, ++$i by $i++, or double quotes by single quotes. Why? Because 99.999999% of the time, it is irrelevant.

"print" uses one more opcode than "echo" because it actually returns something. We can conclude that echo is faster than print. But one opcode costs nothing, really nothing.

I have tried on a fresh WordPress installation. The script halts before it ends with a "Bus Error" on my laptop, but the number of opcodes was already at more than 2.3 millions. Enough said.

share|improve this answer
add comment

In Brief: it Depends

  • Are you really going to need or use your refactored / enhanced version?

    • Is there a concrete gain, immediate or long-term?
    • Is this gain only for maintainability, or really architectural?
  • Does it really need to be optimized?

    • Why?
    • What target gain do you need to aim for?

In Details

Are you going to need the cleaned up, shiny stuff?

There are things to be cautious about here, and you need to identify the limit between what is real, measurable gain and what is just your personal preference and potential bad habit of touching code that shouldn't be.

More specifically, know this:

There is such a thing as Over-Engineering

It's an anti-pattern, and it comes with issues built-in:

  • it may be more extensible, but it may not be easier to extend,
  • it may not be simpler to understand,
  • last, but definitely not least here: you might slow down the whole code.

Some could also mention the KISS principle as a reference, but here it's counter-intuitive: is the optimized way the simple way or the cleany architectured way? The answer is not necessarily absolute, as explained in the rest below.

You Ain't Gonna Need It

The YAGNI principle is not completely orthogonal with the other issue, but it helps to ask yourself the question: are you going to need it?

Does the more complex architecture really present a benefit for you, apart from giving the appearance of being more maintainable?

If It Ain't Broke, Don't Fix It

Write this on a big poster and hang it next to your screen or in the kitchen area at work, or in the dev meeting room. Of course there are a lot of other mantras that are worth repeating yourself, but this particular one is important whenever you try to do "maintenance work" and feel the urge to "improve" it.

It's natural for us to want to "improve" code or even just touch it, even unconsciously, as we read through it to try to understand it. It's a good thing, as it means we're opinionated and try to get a deeper understanding of the internals, but it's also bound to our skill-level, our knowledge (how do you decide what's better or not? well, see sections below...), and all the assumptions we make about what we think we know the software...:

  • actually does,
  • actually needs to do,
  • will eventually need to do,
  • and how well it does it.

Does it really need to be optimized?

All this said, why was it "optimized" in the first place? They say that premature optimization is the root of all evil, and if you see undocumented and seemingly optimized code, usually you could assume it probably didn't follow the Rules of Optimization didn't dearly need the optimization effort and that it was just the usual developer's hubris kicking in. Yet again, maybe it's just yours talking now.

If it does, within which limits does it become acceptable? If there's a need for it, this limit exists, and gives you room to improve things, or a hard-line to decide to let it go.

Also, beware of invisible characteristics. Chances are, your "extensible" version of this code will you up more memory at runtime as well, and present even a larger static memory footprint for the executable. Shiny OO features come with unintuitive costs like these, and they may matter to your program and the environment it's supposed to run on.

Measure, Measure, Measure

As the Google folks now, it's all about data! If you can back it up with data, then it's necessary.

There's this not so old tale that for every $1 spent in development it will be followed by at least $1 in testing and at least $1 in support (but really, it's a lot more).

Change impacts a lot of things:

  • you might need to produce a new build;
  • you should write new unit tests (definitely if there were none, and your more extensible architecture probably leaves room for more, as you have more surface for bugs);
  • you should write new performance tests (to make sure this stays stable in the future, and to see where the bottlenecks are), and these are tricky to do;
  • you'll need to document it (and more extensible means more room for details);
  • you (or someone else) will need to extensively re-test it in QA;
  • code is (almost) never bug-free, and you'll need to support it.

So it's not just hardware resources consumption (execution speed or memory footprint) that you need to measure here, it's also team resources consumption. Both need to be predicted to define a target aim, to be measured, accounted for, and adapted based on development.

And for you manager, that means fitting it into the current development plan, so do communicate about it and do not get into furious cow-boy/submarine/black-ops coding.


In General...

Yes, but...

Don't get me wrong, in general, I'd be in favor of doing why you suggest, and I often advocate it. But you need to be aware of the long-term cost.

In a perfect world, it's the right solution:

  • computer hardware get better over time,
  • compilers and runtime platforms get better over time,
  • you get close-to-perfect, clean, maintainable and readable code.

In practice:

  • you may make it worse

    You need more eyeballs to look at it, and the more you complexify it, the more eyeballs you need.

  • you can't predict the future

    You can't know with absolute certainty if you'll ever need it and not even if the "extensions" you'll need would have been easier and quicker to implement in the old form, and if themselves would need to be super-optimized.

  • it represents, from management's perspective, a huge cost for no direct gain.

Make it Part of the Process

You mention here that it's a rather small change, and you have some specific issues in mind. I'd say it's usually OK in this case, but most of us also have personal stories of small changes, almost surgical-strike edits, which eventually turned into maintenance nightmare and nearly-missed or exploded deadlines because Joe Programmer didn't see one of the reasons behind the code and touched something that shouldn't have been.

If you have a process to handle such decisions, you take the personal edge off of them:

  • If you test things correctly, you'll know quicker if things are broken,
  • If you measure them, you'll know if they improved,
  • If you review it, you'll know if it throws people off.

Test Coverage, Profiling and Data-Collection are Tricky

But, of course, your testing code and metrics might suffer from the same issues you're trying to avoid for your actual code: do you test the right things, and are they the right thing for the future, and do you measure the right things?

Still, in general, the more you test (until a certain limit) and measure, the more data you collect and the safer you are. Bad analogy time: think of it like driving (or life in general): you can be the best driver in the world, if the car breaks down on you or someone decided to kill themselves by driving into your car with their own today, your skills might not be enough. There are both environmental things that can hit you, and human errors also matter.

Code Reviews are the Development Team's Hallway Testing

And I think the last part is key here: do code reviews. You won't know the value of your improvements if you make them solo. Code reviews are our "hallway testing": follow Raymond's version of the Linus' Law both for detecting bugs and detecting over-engineering and other anti-patterns, and to ensure that the code is in line with your team's abilities. There's no point in having the "best" code if nobody else but you can understand and maintain it, and that goes both for cryptic optimizations and 6-layers deep architectural designs.

As closing words, remember:

Everyone knows that debugging is twice as hard as writing a program in the first place. So if you're as clever as you can be when you write it, how will you ever debug it? - Brian Kernighan

share|improve this answer
show 4 more comments

It depends on why the code was optimized and what the effect of changing it would be and what the impact of the code on overall performance might be. It should also depend on whether you have a good way to load test changes.

You should not make this change without profiling before and after and perferably under a load that is similar to what would be seen on production. That means not using a tiny subset of data on a developer machine or testing when only one user is using the system.

If the optimization was recent, you may be able to talk to the developer and find out exactly what the issue was and how slow the application was before the optimization. This can tell you a lot about whether it is worth it to do the optimizing and what conditions the optimization was needed for (a report that covers a whole year for instance may not have become slow until Sept or Oct, if you are testing your change in February, the slowness might not yet be apparent and the test invalid).

If the optimization is rather old, newer methods might even be faster as well as more readable.

Ultimately this is a question for your boss. It is time-consuming to refactor something that has been optimized and to make sure that the change didn't affect the end result and that it performs as well or at least acceptably compared to the old way. He may want you to spend your time in other areas instead of taking on a high risk task to save a few minutes of coding time. Or he may agree that the code is hard to understand and has needed frequent intervention and that better methods are now available.

share|improve this answer
add comment

I will never replace Optimized code with Readable code because I can't compromise with performance and I will opt to use proper commenting at each and every section so that anyone can understand logic implemented in that Optimized section which will resolve both the problems.

Hence, Code will be Optimized + Proper Commenting will make it Readable too.

NOTE: You can make an Optimized Code readable with the help of proper commenting but you can not make Readable Code an Optimized one.

share|improve this answer
show 1 more comment

This is the way I do things: First I make it work in readable code, then I optimize it. I keep the original source and document my optimization steps.

Then when I need to add a feature I go back to my readable code, add the feature and follow the optimization steps I documented. Because you documented it's really fast and easy to reoptimize your code with the new feature.

share|improve this answer
add comment

The main question here is: is the optimisation required?

If it is then you cannot replace it with slower, more readable code. You will need to add comments etc to it to make it more readable.

If the code does not have to be optimised then it should not be (to the point of affecting readability) and you can re-factor it to make it more readable.

HOWEVER - make sure you know exactly what the code does and how to throughly test it before you start changing things. This includes peak usage etc. If do not not have to compose a set of test cases and run them before and after then you do not have time to do the refactoring.

share|improve this answer
add comment

If it's just a short piece of code that does something relatively simple in a hard-to-understand way, I'd shift the "quick understanding" in an extended comment and/or an unused alternative implementation, like

#ifdef READABLE_ALT_IMPLEMENTATION

   double x=0;
   for(double n: summands)
     x += n;
   return x;

#else

   auto subsum = [&](int lb, int rb){
          double x=0;
          while(lb<rb)
            x += summands[lb++];
          return x;
        };
   double x_fin=0;
   for(double nsm: par_eval( subsum
                           , partitions(n_threads, 0, summands.size()) ) )
     x_fin += nsm;
   return x_fin;

#endif
share|improve this answer
add comment

You should not change working, production code without a good reason. "Refactoring" is not a good enough reason unless you cannot do your job without that refactoring. Even if what you are doing is fixing bugs within the difficult code itself, you should take the time to understand it, and make the smallest possible change. If the code is that hard to understand, you won't be able to understand it completely, and so any changes you make will have unpredictable side effects-- bugs, in other words. The bigger the change, the more likely you are to cause trouble.

There would be an exception to this: if the incomprehensible code had a complete set of unit tests, you could refactor it. Since I have never seen or heard of incomprehensible code with complete unit tests, you write the unit tests first, obtain the agreement of the necessary people that those unit tests do in fact represent what the code should be doing, and THEN make the code changes. I've done that once or twice; it's a pain in the neck, and very expensive, but does produce good results in the end.

share|improve this answer
add comment

Basically, you are asking if refactoring is a worthwhile venture. The answer to this is most certainly yes.

But...

...you need to do it carefully. You need solid unit, integration, functional, and performance tests for any code you are refactoring. You need to be confident that they really do test all required functionality. You need the ability to run them easily and repeatedly. Once you have that, you should be able to replace components with new components containing the equivalent functionality.

Martin Fowler wrote the book on this.

share|improve this answer
add comment

The problem here is distinguishing "optimized" from readable and extendable, what we as users see as optimized code and what the compiler sees as optimized are two different things. The code you're looking at changing might not be a bottleneck at all, and therefore even if the code is "lean" it's not even needed to be "optimized". Or if the code is old enough, there might be optimizations made by the compiler to built-ins that makes using a newer simple built-in structure equally or more efficient than the old code.

And "lean," unreadable code isn't always optimized.

I used to be of the mindset that clever/lean code was good code, but sometimes taking advantage of obscure rules of the language hurt rather than help in code creation, I've been bitten more oft than not in any embedded work when trying to be clever because the compiler makes your clever code into something utterly unusable by the embedded hardware.

share|improve this answer
add comment

Think of it from a business perspective. What are the costs of the change? How much time do you need to make the change and how much will you save in the long run by making the code more easy to extend or maintain? Now attach a price tag to that time and compare it to the money lost by reducing performance. Maybe you need to add or upgrade a server to make up for the lost performance. Maybe the product does no longer meet the requirements and cannot be sold anymore. Maybe there is no loss. Maybe the change increases robustness and saves time elsewhere. Now make your decision.

On a side note, in some cases it might be possible to keep both versions of a fragment. You could write a test generating random input values and verify the results with the other version. Use the "clever" solution to check the result of a perfectly understandable and obviously correct solution and thereby gain some reassurance (but no proof) that the new solution is equivalent to the old one. Or go the other way round and check the result of the tricky code with the verbose code and thereby documenting the intention behind the hack in an unambiguous manner.

share|improve this answer
add comment

if profiling shows that the optimization is unneeded (it is not in a critical section) or even has a worse runtime (as a result of bad premature optimization) then sure replace with the readable code that is easier to maintain

also make sure the code behaves the same with the appropriate tests

share|improve this answer
add comment

It's probably not a good idea to touch it - if the code was written that way for performance reasons, it means that changing it could bring back performance issues that had been previously solved.

If you do decide to change things to be more readable and extendable: Before you make a change, benchmark the old code under heavy load. Even better if you can find an old document or trouble ticket describing the performance issue that this odd-looking code is supposed to fix. Then after you make your changes, run the performance tests again. If it's not very different, or still within acceptable parameters, then it's probably OK.

It may sometimes happen that when other parts of a system change, this performance-optimised code no longer needs such heavy optimisations, but there's no way to know that for certain without rigorous testing.

share|improve this answer
1  
One of the guys I work with now loves to optimise things in areas that the users hit once a month, if that often. It takes time and not-infrequently causes other problems because he likes to code-and-commit, and let QA or other downstream function actually test. :/ To be fair, he's generally fast, quick, and accurate, but these penny-ante "optimisations" just make things harder for the rest of the team and their permanent deaths would be a Good Thing. –  DaveE Jun 11 '12 at 17:12
1  
Well, we're going to have to agree to disagree. Spending an hour changing something to save fractional seconds at runtime for a function that executes really occasionally and leaving the code in head-scratching shape for the other developers is ... not right. If these were functions that executed repeatedly in high-stress parts of the app, fine & dandy. But that's not the case I'm describing. This is truly gratuitous code finagling for no other reason than to say "I made this thing that UserX does once a week fractionally faster". In the meantime, we have paying work that needs doing. –  DaveE Jun 11 '12 at 23:58
show 3 more comments

Usually, no.

Changing the code can cause unforeseen knock-on issues elsewhere in the system (which can sometimes go unnoticed until much later in a project if you don't have solid unit and smoke tests in place). I usually go by "if it ain't broke, don't fix it" mentality.

The exception to this rule is if you're implementing a new feature that touches this code. If, at that point, it doesn't make sense and refactoring really needs to take place, then go for it as long as refactoring time (and sufficient testing and buffer for dealing with knock-on issues) is all accounted for in estimates.

Of course, profile, profile, profile, especially if it's a critical path area.

share|improve this answer
2  
Yes, but you assume the optimization was needed. We don't know always know if it was, and we probably want to determine this first. –  haylem Jun 12 '12 at 3:50
1  
@haylem: No, I assume that the code works as-is. I also assume that refactoring the code will invariably cause knock-on issues elsewhere in the system (unless you're dealing with a trivial chunk of code that doesn't have any external dependencies). –  Demian Brecht Jun 12 '12 at 13:56
show 1 more comment

Where?

  • On a home page of a Google-scale website, it is not acceptable. Keep the things as quick as possible.

  • In a part of an application which is used by one person once a year, it is perfectly acceptable to sacrifice performance in order to gain code readability.

In general, what are the non-functional requirements for the part of the code you're working on? If an action must perform under 900 ms. in a given context (machine, load, etc.) 80% of the time, and actually, it performs under 200 ms. 100% of the time, sure, make the code more readable even if it might slightly impact the performance. If on the other hand the same action never performed under ten seconds, well, you should rather try to see what's wrong with the performance (or the requirement in the first place).

Also, how readability improvement will decrease the performance? Often, developers are adapting the behavior close to premature optimization: they are afraid to increase the readability, believing that it will drastically destroy the performance, while the more readable code will spend a few microseconds more doing the same action.

share|improve this answer
40  
+1! If you don't have numbers, get some numbers. If you don't have time to get numbers, you don't have time to change it. –  Tacroy Jun 11 '12 at 19:27
41  
Often as not, developers are "optimizing" based on myth and misunderstanding, for instance, by assuming that "C" is faster than "C++" and avoiding C++ features out of a general feeling that things are faster without numbers to back it up. Reminds me of a C developer I followed who thought goto was faster than for loops. Ironically, the optimizer did better with for loops, so he made the code both slower and harder to read. –  Steven Burnap Jun 11 '12 at 19:35
4  
Rather than add another answer, I +1'd this answer. If understanding code fragments is that important, comment them well. I worked in a C/C++/Assembly environment with legacy code a decade old with dozens of contributors. If the code works, leave it alone and get back to work. –  Chris K Jun 11 '12 at 20:31
show 1 more comment

In general, you should focus on readability first, and performance much later. Most of the time, those performance optimizations are negligible anyway, but the maintenance cost can be huge.

Certainly all the "little" things should be changed in favor of clarity since, as you pointed out, most of them will get optimized by the compiler anyway.

As for the larger optimizations, there might be a chance that the optimizations are actually critical to reaching reasonable performance (though this isn't the case surprisingly often). I would make your changes and then profile the code before and after the changes. If the new code has significant performance issues, you can always roll-back to the optimized version, and if not you can just stick with the cleaner code version.

Change only one part of the code at a time and see how it affects performance after each round of refactoring.

share|improve this answer
add comment

Your Answer

 
discard

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.