First of all, I'll assume you're not an idiot. If you're an idiot, and your task requires you to interact with this code, you're fucked no matter what.
So, assuming you're not an idiot, and you've asked the question, means you're already pretty damn confident this code's not on a critical path. And what does that mean? Means some asshole was showing off how clever ey was by prematurely optimizing, and it's already come back to bite you in the form of difficult maintenance. So take a blowtorch to it already.
Most software engineers, smart or not, do not have the skill or experience to even analyze the intent of highly-optimized implementations. The complexity is obvious but the interactions being manipulated are not unless software system optimization is a domain of expertise. It would be like asking me to analyze GUI design.
I have seen many examples of smart engineers being "damn confident" that code was over-engineered and possibly even suboptimal only to make the code simpler, sometimes even faster, and broken. I design high-performance software systems and am the optimization guy. There have been many cases where a smart engineer decides that I am actually "some asshole showing off how clever ey was" and then "fixes" my work. Which then has to be redone when the purpose of the original implementation rematerializes.
The engineers in these cases are not idiots, they simply don't understand the nature of the optimization or why it is happening in a particular place. There are many cases of premature optimization but that is not always the case just because it looks like it. If there are people around that actually understand optimization then at least ask one of them about the code in question, particularly if they wrote it.
Exactly. It's too easy to break Zawinsky's first commandment (Thou shalt not break working code) with changes like this. Sure, if the code is getting in the way of a change, then go ahead and change it. But I don't think that's what the questioner is asking. It seems like he or she has noticed this "ugly" code and wants to change it out of a misplaced sense of conscientiousness.
EDIT: I just remembered that something like this happened to me when I was an intern at my first position doing "real" software development. I'd noticed a particular piece of gnarly code and asked my team lead for permission to refactor. He, in turn, told me that for a rarely used code path like the one I was proposing to refactor, the costs of QA and deployment would greatly outweigh any savings my changes would generate for the company. His concluding words were, "Sometimes it's easier to let sleeping dogs lie," and I think that's something that applies to this situation as well.
> 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.
Last paragraph:
> Is it a good idea to change such code if you have to touch a small piece of it either way?
If the intent of those code optimizations are so damn hard to understand and there are such subtle bugs they avoid in very specific ways, why isn't there a comment to explain the intent and a unit test to ensure it's not broken?
Sounds like it's not those other engineer's fault that this kind of thing keeps happening to your code...
The code is perfectly clear and well-documented. In most of the cases, the problem is that most software engineers have no clue how the hardware (or operating system) their software runs on actually works. Consequently, they are oblivious to potential side-effects of innocuous-looking code changes that the original author accounted for. There is always documentation explaining the optimization and intent but many engineers do not have the background knowledge to understand how that is achieved. The code optimizations are easy to understand if you know how to optimize code. The problem is that most software engineers do not, nor do they know what optimizations look like even when the code itself is easy to understand. So the best we can do is annotate that code with the fact that some important optimizations were made there and to understand the nature of those optimizations.
Also, it is difficult to impossible to unit test many optimizations. Many of them are about minimizing interactions between concurrent but unrelated processes or minimizing the latency of some code path. The functionality can be correct and fast in unit tests and the run-time behavior in operational environments still be pathological because of potential interactions that are nearly impossible to replicate in the unit test environment. You can run simulated workloads but those are much more expensive to do and for any kind of non-trivial system the coverage will be poor relative to the kinds of problems you may have.
99% of optimized code I saw in my life is optimized as result of process called "performance decoration": optimizing piece of code based on some (mainly faulty) assumptions without tests and profiling.
I think "performance decoration" is defined as a process in which you make software to look fast but without any positive impact on actual delivered performance (there was an article in ACM a while ago about it).
For example, I saw heavily optimized (and convoluted) C++ loops inside I/O heavy code where just increasing block size and enabling direct IO gives 90% performance boost.
So if there is no "performance lifecycle management" explanation related to optimized code (i.e., which test needs to be run, what is dependency, etc.) then the code is probably just "performance decoration". My theory is: if somebody did investigation to show that this is a valid performance optimization he would probably document all needed tests which led him/her to that conclusion.
Stealing is if you like some term and start using it with a different definition than the author. Like Service Oriented Architecture (I have web services, so it's SOA) or Technological Singularity (e.g. I'll just call the Neolithic and Industrial revolutions singularities).
I always optimize new code for readability. If the old code is on a hot path, I'd suggest adding comments explaining why did it end like it did and leaving it alone (unless you readable version is equally fast). If it's not and it's structure starts to interfere with maintenance, replace it with simpler code.
Of course, before you touch it, make sure it's adequately tested and, if not, make it adequately tested.
If you ask this question, then no. Otherwise, you know the answer.
Further explanation on #1 You lack the experience required to understand the ramifications, make the judgement, change the code. Gain experience, learn more about this codebase and you will "know" one day.
Given that, one of the "best" ways to gain experience is to make and suffer through recovering from mistakes, change away!
I was going to write a detailed explanation of the trade-offs and cases where you might want to make such a change, but turns out, the article did an excellent job of hitting that question from all conceivable sides. Really quite good.
Are you touching the code because you need to fix a bug in it? If so, then yes.
If you need to maintain this piece of code then you might as well fix it to the best of your ability and knowledge, but please be careful.
Are you touching the code because you came across it, and you don't like how it looks? If so, then no.
It's a shame that unreadable code exists, but you can't save the world. My mom used to tell me "Don't sit in someone else's pile of shit." Meaning, if someone else made a mess, and if you're the last person to touch it, you're going to look like the one who did it. There are bigger fish to fry in any software project than shitty-code-that-hasn't-been-touched-in-a-while-that-works.
You realy have to ask why it was optimized in the first place. Now if that optimization has been negated by the enviroment then you can ask yourself the next question. Does the code need to be or envisaged in the near future to need changing for a reason other than making something that works into something that works and is readable but not as fast.
Both question are important and if you answear no to either then your best leaving the change to a time when you have to change it. Reason being that you may have a margin in the enviroment negating the optimization today, but that may change before you need to change the code for a reason of business. You could end up beutifying it and losing that optimization and then end up hitting that wall a few months/years down the line and have to reinvent that optimization.
Over all the most important aspect about reading code is to understand the structure and by that this part does this and that part does that. If your calling a optimized load of code in a function then if that code is sexy looking or not it will still have the same look from anything that calls it.
So as a rule, if you don't have to touch it and don't need to touch it then ask yourself why your touching it and are you workbombing yourself with something in a less busy period that will drag on over into a busy period and cause you regret it down the line. Also make sure your doing it for the right reasons, beyond putting a flag ontop of a mountain so to speak, what are you gaining. If something works and does not need to change then changing it just to make it pretty is like arguing all programmers should be wearing size 0 dress's to many peoples minds.
Over and over again in my career I run into scenarios where code maintenance is the biggest challenge. Only occasionally do I run into issues with code performance. But, I mainly work on business type apps. If I were doing real-time or embedded systems with limited resources I'm sure my experiences would be totally different.
The best thing is to replace the unreadable code with readable code while preserving or improving its performance characteristics.
Optimized code is often unreadable because the initial implementation didn't forsee the future optimization. A re-write that takes both into account at once is usually at least as fast and much more readable.
Communicate: ask your team to find out who wrote it, and why. Ideally you're using git and can use git blame to find the author. Benchmark the difference, because maybe there's more going on that you expect at first.
From the article: This Q&A is part of a biweekly series of posts highlighting common questions encountered by technophiles and answered by users at Stack Exchange, a free, community-powered network of 80+ Q&A sites.
It's not as if they're trying to hide the source, and the content is presented in an easier to read format than SE. The only question one might ask is why most of hansbo's submissions are for arstechnica.com articles.
Stack Exchange submits these to Ars (or at least, has some sort of deal), everything is linked off to Stack Exchange, and it says Stack Exchange all over the page. I dunno if you can call it 'ripped off', just because someone didn't submit the original.
If you're asking, then YES, DO IT.
First of all, I'll assume you're not an idiot. If you're an idiot, and your task requires you to interact with this code, you're fucked no matter what.
So, assuming you're not an idiot, and you've asked the question, means you're already pretty damn confident this code's not on a critical path. And what does that mean? Means some asshole was showing off how clever ey was by prematurely optimizing, and it's already come back to bite you in the form of difficult maintenance. So take a blowtorch to it already.