Special Cases Are a Code Smell
blog.conjur.org
blog.conjur.org
Two months later the law changed adding a new special case somewhere deep inside. I could start over. Lesson learned: don't outsmart the logic of the business.
Very often when people try to solve problems, like the one you faced, with software, they fail to recognize that you might have to rethink the business logic. It's my belief that the majority of software design for specific niche business and governments fail because there was a lack of willingness to change and simplify the rules.
Of cause a project create a new system for taxation will fail, if you have 6000 pages of rules, not including the tax law itself.
Often we think we have a software bug/issue, in reality what we have is a flawed business logic. Until such time the people in charge fixes the logic, we're not able to do anything clever of efficient in software.
“It was decided!”
“By whom? Why?”
“???, It was decided!”
“...”
The logic is that often the specs don't change, and if you make the code concise enough, you can just rewrite it if you need to make it more general.
If it was easy, developers wouldn't be well paid.
Create a system that handles all the rules of a single municipality or so -- doable. But too expensive for a single municipality.
So we create one system that will work for all municipalities! That way they can afford it, right?
Yes, but none of them is going to change their process. All the different ways of doing more or less the same thing have to be supported, in one system.
End result: nothing working, way over budget. Every single time.
Oh, we often recognize this.
You try telling a paying client that you think their business process is sub-optimal. I'll fetch the popcorn...
There are systems out there essentially replicating the workflow of older systems which replicate the workflow of an even older one, which was designed simply to automate a paper-based process all those years ago. If you are talking to a large business then the chances are that the people you are talking to don't have authority to change the business processes even a little bit.
This way you know when you come back to it, what must stay, what must go, and what must change.
Hats off to you, you wonderful person!
This resulted in him filing a large number of minor bugs between the spec and the reference implementation: https://hevc.hhi.fraunhofer.de/trac/hevc/query?status=accept...
And a product: http://www.argondesign.com/products/argon-streams-hevc/ (page includes explanatory video)
Edit: the tool critical to the project was "Ometa": https://en.wikipedia.org/wiki/OMeta
On a much smaller scale, VPRI made part of their TCP stack by parsing ASCII diagrams of packet contents (e.g. see the STEPS reports at http://vpri.org/writings.php )
Edit: Ah, I see user nradov has already mentioned this in a sibling comment :)
http://www.vpri.org/pdf/tr2007008_steps.pdf (Appendix E).
There is a reason people tell you to do "KISS". Which is just coding what you need right now, and not trying to be too smart about it.
Life is full of special cases : try to code an international calendar application, and you'll see what I mean. So many cultural things, so many political things, so many legal things...
So yeah, if you can find an elegant way to generalize your problem, do so. But unless you work on a purely theorical topic, you will have special cases. And they will change.
If you can get over their lack of coolness (some implementations use spreadsheets!?!?!) they are a nice way to separate the crazy complicated stuff that changes all the time from the rest of the more static logic.
It's easier to check that the code matches the law (after all there are no test vectors on the law apart from case law so you have to go by the drafting) and it makes updates when an amendment is made much simpler.
That, and there is usually zero need to optimise.
Every year on budget day I used to listen to the live speech from the house of commons in case they changed something that effected billing - one year they changed VAT (sales tax) in the middle of the month.
Anyway, business logic is a perfect example of stuff that should go in Policy: functional, side-effect-free code (but not necessarily tidy or easy to read) which computes a value from a set of inputs. Like the parent says, effort spent optimising here may end up being a poor investment if the requirements change every year.
Mechanism code, OTOH, generally ends up containing all the side effects. It's harder to test so it tends to be dumb and, therefore, less susceptible to changing requirements. Optimisations here can pay off because the code lives longer.
Really though it all comes down to tests, tests and more tests. Automation is an asset and test cases outlive everything so make sure they work against the interface, not the implementation.
Time for a DSL and a compiler from the DSL to a truth table! :)
Rejiggering an algorithm so as not to have special cases just puts cognitive load on whoever reads the code to suss out what the original algorithm was. "Why does this algorithm require its input array to be padded with zeroes? Oh I see, it doesn't - the developer just did that to remove some if statements."
Sometimes that's how it goes, but other times the special cases were a result of just not knowing the original problem well enough to think of a clean solution that really captures its essence, and the special cases are evidence of a sort of resultant duct taping.
Agreed though, what you're describing definitely happens and I'm not a fan.
- code for left edge
- code for body
- code for right edge
Conditionals are great because they're clear and concise...
if not list: # Can't do anything with this
return # or raise error
if len(list) < 3: # Ditto
return #ditto
I'm sure special case elimination is useful and sometimes necessary for clarity, but only if not done at the expensive of said clarity.
I would be open to seeing some examples that make clearer sense, though. A video I found much more convincing was Embracing Algorithms[1] from Apple's WWDC 2018. I just watched it last week, and I found the ways it broke down and deobfuscated rather simple problems into easier solutions that describe the problem and solution more concisely to be superior to what was in the article.
I think a suggestion to think about your reader foremost resides at the core of your comment, and I think that’s what’s most important. In a context that’s predominantly iterative or object oriented you’re completely right—the special case pattern is far more pervasive and you’d be doing your readers no favors by abandoning it. However if you could expect your readers to come equipped with a host of functional idioms, the elimination suggestion isn’t so bad (at least from a semantic/comprehension standpoint, ignoring performance).
Yes, you can sometimes rewrite your algorithm to eliminate "special cases", but if you do it at the cost of comprehensibility, it's not a net gain. In the case of these examples, we're not really even talking about "special cases", but edge cases, where you literally need to deal with an edge in the data. In the case of the neighbor sums, edge cases were masked by padding. In the case of the staircase, the edge is just obfuscated by the doubled labels and then hidden in a modulo operation. These are totally fine except that they seem to provide no value except "elegance", and they come at a high cost in terms of understandability. (The first step is literally "transform to a different problem", so now you need to understand the original problem as well as the new problem, and how they relate.)
I'm all for elegance when it comes bundled with understandability or legibility or performance or even simplicity. But often it actually means "clever" and it's an extremely subjective measure that delivers little to no value and is prone to causing problems later.
The version with special cases feels cleaner to me.
Indeed. This article has a bit of a smell. Sacrifice performance to the almighty gods of your opinions about elegant code? It doesn't make code less buggy. It verges on a holier-than-thou attitude, that case-y code is Bad(TM), with a sequence of increasingly-contrived examples.
And in fact, lots of algorithms are unavoidably case-y. Teaching programmers to sneer at bounds-checking, as this article does, is downright irresponsible. Sure; the right language might do that for you -- but whether it's a default value or a runtime error, a programmer's failure to fully explore the edge cases will result in incorrect behavior, crashes or both.
Pretending like cognitive load is never worth the effort is wrong.
There's complexity inherent to the problem and complexity that's incidental to the problem.
Cognitive load inherent to the problem should be isolated.
Cognitive load required to deal with incidental complexity (such as clumsy abstractions, etc) is a waste of time and should absolutely be minimised.
In the particular case you mentioned it's perfectly reasonable to explain why the array is getting padded in a code comment.
Either way, the example implementation assumes the first and last elements are treated as having only one neighbor; which seems like an odd specification for the algorithm in the first place. To me it seems like the source of this code smell is a bit further up in the call stack.
As for the second example, it becomes even more obvious how inefficient the "better" solution is. Just copy the entire array except the last element, in reverse order no less (so it can't just be memcpy()'d). This is what happens when you teach kids Ruby (or php, or pythin or...) because the job market demands it, but not the fundamentals (i.e. C, C++, Pascal, etc.).
As for elegance, both "improved" examples seem extremely ugly to me. Not only do they turn a clearly readable if statement into something more magical, they also mutate their data before operating on it. Turning 5 lines of structured code into a one-line abomination isn't elegant, it's dumb. It decreases maintainability. It increases complexity. It makes room for errors to hide. It's, to put it simply, bad coding style.
The idea to not prematurely optimize was never intended to be a free pass to use wasteful practices everywhere until there’s a horrible problem, it’s more about avoiding factoring your code into something you can’t maintain. Generally speaking, baking your corner cases into your precondition code is more dangerous than avoiding allocations.
The idea was also not to leave concerns about performance completely off the table until your application is correct; it is assumed that you are making somewhat reasonable choices for your unoptimized code and your architecture, not that you are actively trying to slow things down with unnecessary allocations.
Elegant code uses less instructions, clear structure and is in general one of the better implementations in terms of performance. The suggested implementation shifts the workload into standard ruby functionality, but multiplies it by many times in the process; turns the clear structure of two conditionals into weird data trasnformations that and ultimately performs much worse than just the two extra ifs.
The multi-gigabyte-dataset was obviously an extreme example (yet one that may very well happen in practice). The point is, bad code like that is inevitable going to slow down an application, the question is whether you care about speed at all. Arguably, if you're using ruby, you probably don't.
Premature optimization usually refers to taking straight-forward code and making it more complex based on certain performance assumptions, like unrolling loops or writing inline assembly. This is bad because, well written code is most of the time fast enough, and if it does need optimizations, that will become clear soon enough.
Choosing not change your code in a way that considerably impacts performance based on ones individual perception of cleverness and elegance, however, is not an optimization; it's avoiding a dumb mistake that will bite you in the ass later on.
E.g. in C# using Linq:
foreach (var num in myArray.Prepend(0).Append(0)) { /* blah */ }I'm not perfect either, but that deficiency for the sake of elegance is technical debt. A year later, when that piece of code is running thousands of times over thousands of numbers, nobody is going to know why it does what it does. Just that the system throws random out of memory errors or something.
The IF statements are not even necessary. Compute the sum for the first elements neighbors first (sum[0]=element[1]) and the last element (sum[n-1]=element[n-2]) Then loop over the remaining elements with no conditionals.
I agree with the author that it's probably better to pad it than to have conditionals at every iteration, but for this problem there is no need to have a conditional at all.
"With enough 'ifs' you could Paris in a bottle."
This is what we, as software developers, do! We create conditions to make (amazing) things happen!
Conditionals are the bread and butter of programming. They're what allow us to "fix" things in short order. They give us the flexibility to make our software work in a seemingly infinite number of situations.
They also make debugging easier! Waking through the logic of even a zillion conditionals is quite straightforward and gives you that, "aha!" moment pretty quickly. Whereas walking through an algorithm that only works on a weirdly transformed data set just has us scratching our heads wondering why the previous developer felt the need to, "fuck with the data" instead of just writing a few clear and concise conditionals.
Conditionals are also easy to split up! They give you lots of clear options if they start to "grow out of control" as it were. Whereas transforming the data in various code paths can rapidly create debugging nightmares!
> There are two ways of constructing a software design: One way is to make it so simple that there are obviously no deficiencies, and the other way is to make it so complicated that there are no obvious deficiencies.
I hit this problem the other day, writing some code to divide a set of items evenly into N subsets. I wrote a bunch of property tests for this, but kept having problems with the way it was handling remainders. I could have made the tests pass by adding a few more conditionals, but instead I tried to solve it using no more than one conditional plus a handful of arithmetic operations.
The reason I constrained myself was that those test failures were a precious resource to help me understand the problem. Adding complexity to the solution could have produced a correct implementation, but I felt it would more likely out-smart my test suite. I would rather work with something that's clearly broken in an obvious way, rather than possibly broken in an unknown way.
Eventually, I came to realise that the requirements I'd come up with were inconsistent!
- Alan Perlis
The special cases: they are not removed, merely transformed from `if` statements to boundary conditions on the iterator. The `i == 0` and `i == input.size - 1` checks effectively move into the iterator, with how the nice and clear `input.map.with_index do |_, i|` becomes the magic `(1...padded.size-1).map do |i|`. The 0 added to the start and end are also easily arguably special cases.
The second form is almost certainly less efficient because you’re creating at least one new array, unnecessarily.
> The special cases are gone, and the simple algorithm shines through.
The algorithm does shine through more, but the special cases are not gone, merely transformed (and into an arguably less clear form), and the code is less efficient.
Simplifying things is a worthy goal, and a lot of special cases are code smells, but be careful in seeking to remove them, and remember to at least consider performance, even if you end up deciding to prefer succinctness instead.
I think you’re too quick to assume that your preferred solution is faster.
Personally, I’d use the more clear code and only if the code proved to be a bottleneck even consider refactoring for performance. Whether the author’s preferred solutions are clearer, I’m not convinced however.
Finally, the author doesn’t claim that the special cases are gone, but more so that the input is transformed in a way to allow processing that is blind to them.
Not very familiar with the language, but the code appears to create a new array by concatenating three arrays, which means copying all the elements; how is that O(1)?
Coirdially yours,
falsedan
#whoa
I understand that reading is hard and avoiding needless posturing in order to display a personally-appraised superior intellect is harder, but reading in good faith requires that you put in some effort and not knee-jerk
But what you call “the O(1) operation of adding two items to an array” is simply not true:
① While appending to arrays is normally O(1) in the cases that there is still spare capacity in the array and the entire thing doesn’t need to be reallocated, prepending is O(n) as it has to move all of the subsequent values along.
② What’s happening is not just adding two items to an array, but rather allocating an entire new array and copying all the values into it; this will be roughly O(n), but with a high constant overhead: allocating memory is expensive.
I can’t speak of any Ruby interpreter’s performance on the matter, and I’m fairly confident that the conditionals will be much more expensive there than they would be in Rust (where I have no doubt at all that the conditional version would smoke the allocating version in all cases), but allocating memory is a pretty expensive business.
> Finally, the author doesn’t claim that the special cases are gone
I quote: “The special cases are gone”. Further, I do not grant your response that the processing is blind to them—the inside of the loop is, but the special cases were moved to the boundary conditions of the loop. They’re still there.
[1] https://stackoverflow.com/questions/8353026/what-is-the-run-...
No, you still need to copy the old array to the new array.
FWIW, Ruby may already be allocating some space before and after the array to accommodate a few pre-/appendages.
That's just a lock (nontrivial but O(1)) and a memcpy (technically O(n) but trivial, and O(1) for the common case if it's implemented with vector instructions), plus in any event the sums-of-neighbors method has to be at least O(n) on an idealized Von Neumann machine because it must read every element of the source array and also write every element of the destination.
> technically O(n) but trivial
"Technically" O(n) is the only O(n). There isn't some widespread colloquial use of Big O notation where O(n) means something else. Whether it's trivial is beside the point, but for a large n, O(n) in both time and space can be prohibitive, and it may become important that I don't use such an algorithm. For example, if I have 8 GB of data and 12 GB of working memory I can't satisfy those space requirements.
> and O(1) for the common case if it's implemented with vector instructions)
What is the common case in your view? memcpy in the general case is O(n). That you can perform multiple copies in parallel might affect the real time, but it doesn't affect the time complexity because O(kn) = O(n) for a constant k even if that k = 1/16 or however many copies you can perform at once.
> plus in any event the sums-of-neighbors method has to be at least O(n) on an idealized Von Neumann machine because it must read every element of the source array and also write every element of the destination.
O(3n) = O(2n) = O(n)
In idealized algorithmic analysis, but not necessarily real life. "Amortized O(1)," which I assume you concede is a commonly-used, meaningful and legitimate term, means "technically" an idealized O(>1) but O(1) in practice.
Calling memcpy inside a Ruby method call is amortized O(1) because for any "n" that fits within available memory, it will always be much faster than the other things in a Ruby method call, which involve dozens of locks, hash table lookups with string keys, dynamic type checks, additional Ruby method calls and so forth.
Likewise, computational complexity on an idealized Von Neumann machine isn't always the same on a real computer, in both directions. Dynamic allocations are theoretically O(n) but may be O(1) if the program never exceeds the preallocated space. Or suppose there were a loop over an array of pointers which dereferenced each pointer; the dereferences are theoretically O(1) but may be O(n) if they evict the parent array from the cache.
> What is the common case in your view?
Such as an array small enough that it can be copied with 10 or fewer vector load/stores.
> O(3n) = O(2n) = O(n)
Yes, that's my point. It's impossible to implement the example in less than idealized O(n) time, so O(n) and O(1) operations are equivalent complexity-wise WRT the entire method.
Big O notation is used for idealized algorithmic analysis. If you want to talk about real life, you can count cycles, seconds, watts etc.
> "Amortized O(1)," which I assume you concede is a commonly-used, meaningful and legitimate term, means "technically" an idealized O(>1) but O(1) in practice.
Yes, but I wouldn't take O(1) on its own to imply amortized complexity. Not that pretending that an array copy is O(1) in practice is particularly useful here since if you measure a copy operation in practice, you'll find that the time it takes scales roughly linearly with the size of the array. Not to mention that the space complexity is O(n) no matter how you put it.
> Such as an array small enough that it can be copied with 10 or fewer vector load/stores.
Are other cases conversely "uncommon"? My point here is that this is entirely your opinion and doesn't pertain to whether an array copy is O(1) or O(n) complex.
> Yes, that's my point. It's impossible to implement the example in less than idealized O(n) time, so O(n) and O(1) operations are equivalent complexity-wise WRT the entire method.
Not in terms of space.
the "ifs" inside the loop will be correctly predicted for cases 2..999,999
the processing is not "blind" to the new special case: "the array is padded", the loop is 1..size-1, which I think it's an error. Shouldn't be 1..size-2?
1...size-1 == 1..size-2
Adding an item to each end of the array induces two problems:
- Adding to the beginning of the array will require all elements of the array to be shifted, and possibly a reallocation and cache miss (depending on implementation language).
- Adding to the end of the array will require traversing the array, potentially causing an extra cache miss.
Cache misses and reallocations are generally the highest performance costs to pay in computing today. O(x) alone is not sufficient to determine an algorithm's real-world performance.
No, it's not, but in my opinion not because of any of the reasons you just stated. Unless you're writing Go, that code should have been a fold over addition, or a `sum` function if available in the standard library.
class Array
def neighbor_sums
map.with_index do |_, i|
fetch(i - 1 & 0xFF_FF_FF_FF, 0) + fetch(i + 1, 0)
end
end
end
[1, 1, 1, 1].neighbor_sums # [1, 2, 2, 1]Represent the question faithfully first, then optimize later maybe sometimes.
result = input.map.with_index do |_, i|
input.better_fetch(i-1, 0) + input.better_fetch(i+1, 0)
end.to_a input.get(i.wrapping_sub(1)).unwrap_or(0) + input.get(i+1).unwrap_or(0)
This is one area where Swift's choice of using signed Int for most things actually works quite well. The equivalent Swift code would be something like
(input[safe: i-1] ?? 0) + (input[safe: i+1] ?? 0)
Not in release mode.
result = input.each_cons(3).map do |left, _, right|
left + right
end
Edit: nevermind, boundary conditions are still special cases.
Relatedly, in image and video processing the (literal) "edge cases" are sometimes handled by allocating the buffer slightly larger in the first place, so that filtering operations (which are very similar to that example) don't run off the edge. Maybe the real lesson here is "think ahead"?
remove_list_entry(entry)
{
prev = NULL;
walk = head;
while (walk != entry) {
prev = walk;
walk = walk->next;
}
if (!prev)
head = entry->next;
else
prev->next = entry->next;
}
remove_list_entry(entry)
{
indirect = &head;
while ((*indirect) != entry)
indirect = &(*indirect)->next;
*indirect = entry->next;
}
===
The physicist is showing his friend the programmer a thermos.
"You see, you can put a hot drink inside, and then take it with you. It doesn't matter how cold it gets outside; when you pour the drink out it's still hot!"
The programmer is quite impressed.
The physicist continues, "Or you can put a cold drink inside, and then take it with you. It doesn't matter how hot it gets outside; when you pour the drink out it's still cold!"
Now the programmer is really dumbfounded.
He asks, "But how does it know?"
===
Unfortunately though I like the joke, I still see waaay too much code written by the programmer in the joke.
But a thermos with a switch on the top to say "insert hot" or "insert cold" would be absurd. A thermos is simply a container _with no concept of temperature_, just isolation: it just maintains the state blindly; whether that state is hot or cold is managed by the "caller" who inserts and extracts the contents, inspecting them itself if it wants to know what had been put into it.
Yet I still see plenty of code that does have a variable to indicate what's inside, perhaps an enum {hot, cold} or worse a bool. Yuck -- you should simply store the thing and if you need to know its state you look at it. That variable is something to get out of sync; something you need to be careful to set and maintain, something at threat to a race condition or ill-timed interrupt. Bleagh. If your first language was FORTRAN, OK. Otherwise there is no excuse.
Sadly I see shit like that in pedagogical examples too, which is malpractice.
Now I'm confused... How are those two things different? The variable indicating what's inside is going to be coming from the actual state itself, no?
It sounds like perhaps you're talking about some case where a developer has somehow copied a state and referred to the copy somewhere else. Or are you talking about a computed variable of some part of the state being a code smell?
if (condition == True)
return True
else
return FalseOf course, the answer is "the code should know beforehand what's in there". If someone hands you a thermos with no clue what's inside, then you either have to assume it could be hot OR cold (ie. just use the base Drink * pointer) or you have to check the temperature (ie. explicitly check the type of the return pointer using runtime type information or some kind of Variant-style wrapper.
The second is significantly 'cleaner,' but with pretty bad readability [0]. The first version has excellent readability.
In practice, the way I'd evaluate the two is by looking at how much other code depends on (or at some point may depend on) the piece under consideration. If nothing else or only very little depends on the code, I'll opt for readability; if it's likely to be somewhat foundational and other stuff is going to grow on it, I'll opt for the more abstract, cleaner version, and document thoroughly.
[0] I'm guessing the readability isn't nearly as bad if the first example is using a common C idiom; I don't use C enough to know. My main criterion for judging readability is: how much does the structure of the code match the conception of the algorithm you're starting with. The conception here has to do with navigating links; the pointer stuff is incidental complexity.
The main problem with the last one is that it's not immediately obvious what the "indirect" represents. When it clicks that it is "the pointer to potentially update", it's pretty easy to understand.
Though I think putting the head check at the beginning like you said will make the first example very reasonable (and relatively compact too).
remove_list_entry(entry)
{
if (head == entry) {
head = entry->next;
} else {
prev = head;
while (prev->next != entry) {
prev = prev->next;
}
prev->next = entry->next;
}
} while(p->next != entry)
p = p->next;
p->next = entry->next;
struct Node
{
struct Node* next;
};
struct DataNode
{
struct Node* next;
struct Value value;
};
struct List
{
struct Node dummy;
// You can also add length, etc.
};
// entry must be in list
void remove_list_entry(struct List* list, struct Node* entry)
{
Node *prev = &list->dummy;
while (prev->next != entry) {
prev = prev->next;
assert(prev != NULL);
}
prev->next = entry->next;
free(entry);
} struct entry { int value; entry * next; };
entry * head = NULL;
void remove_entry(entry *target) {
long offset = (long)&((entry*)0)->next;
entry * curr = (entry*)((long)&head - offset);
while (curr->next != target)
curr = curr->next;
curr->next = target->next;
}
>A pointer to a structure object, suitably converted, points to its initial member
struct entry { entry * next; int value; };
...
// virtual entry where &(curr->next) = &head
entry * curr = (entry*)&head;
*curr = curr->next
indirect = curr
*indirect = *curr
*indirect = curr->next
indirect = &(curr->next)
C99 standard: http://www.open-std.org/jtc1/sc22/WG14/www/docs/n1256.pdf
In this case, it is putting code elegance over readability/maintainability. Sure, you've reduced the number of if statements, but those if statements explicitly represent the logic behind the calculations. Come back 6 months later, and you'll be scratching your head about what's going on. Unless you add a comment, but comments are also a code smell.
Not having comments is a code smell. Everything has context and it is wrong to assume that the next developer to come along will know/understand it well (or at all). Even you as the original coder might not remember why you did something a certain way.
I personally comment like I'm going to start suffering from a massive cognitive decline any day now and will need most things re-explained to me.
Example: I was looking at some old code this morning...
temp.write('\n')
temp.close()
# Add a trailing newline so 'cat' doesn't leave an ugly mess
The need for comments means the code is not clear. That need is a code smell.
Needing comments to explain what the code does is a smell.
Needing comments to explain why the code needs to do what it does is not a smell.
Code smell is just another way to compare two possible approaches. If one version can include significantly fewer comments without issue then that’s good sign.
Sure, and if all the code you ever write is implementing some trivial school assignment then your probably fine. No one with half a brain thinks your example requires a comment, but it's a bad example.
Sure, the functionality to find that SalesTax number might take tens of thousands of hours to create and cover a multitude of egde cases. Still with proper context, structure, naming conventions, etc the why’s should be clear. If your thinking “The exceptions function adjusts for tax holidays. So of course it needs to get exceptions based on location and the order date, and then it needs each items metadata etc” then that’s a great sign.
Elegant code is elegant when it encapsulates the why’s not just the how’s.
Sure, and I'd call that bad code unless it exists because there are far more considerations than sales tax. Either way I don't see how that is an example of when to or not to leave a comment.
>Sure, the functionality to find that SalesTax number might take tens of thousands of hours to create and cover a multitude of egde cases. Still with proper context, structure, naming conventions, etc the why’s should be clear.
Again, that's just not true. I have a hard time imaging that you're a professional engineer with real world experience if you've never found yourself in a situation where variable names alone could not express the _why_ behind a piece of code.
>Elegant code is elegant when it encapsulates the why’s not just the how’s
Great, not always possible. For example:
// We have a longer than normal backoff period on
// timeouts here because device XYZ is a piece of junk
// and randonly stops responding for minutes at a time
// version 2 of the spec switched to an XML format and
// allows the header to be anywhere above the root
// element of the document (as a processing
// instruction). We cannot define a reasonable min
// header position/length. Just read the whole file.
const size_t MinHeaderLength = std::numeric_limits<size_t>::max();
// Workaround issue caused by .NET 4.6.1 upgrade which
// has more restrictive certificate checks for secure
// connections. This is currently affecting SignalR
// Scaleout connections to Azure Service Bus.
AppContext.SetSwitch("Switch.System.IdentityModel.DisableMultipleDNSEntriesInSANCertificate", true);
Several years ago I was updating this ancient Object Pascal program. OS X had just showed up and they finally decided to do a full rewrite, but they wanted to do this is stages. Anyway, this thing was still using Apple Talk networking not TCP/IP and I was rewriting the network stack so we could replace each system individually. Surprisingly, the code was very easy to read, but was also filled with a long legacy of past issues. Comments on 68000 processor issues could safely be ignored for example. So yes lots of comments, but most of them had become useless.
More recently, I was redeveloping a .NET website that had been built by someone in love with XML. Unfortunately, the mismatch between the way the code operated and the way the system operated meant that you needed to carefully read each comment. It had slightly fewer, but far more nessisary comments which was one sign among many that it was a horrible design.
Which is why I am talking about nessisary comments. Many comments can safely sit in source control or automated tests. Their context quickly becoming outdated. However, when a systems design nessitates a great many important comments that’s a bad sign.
The biggest problem with comments is they are not part of the tested integrity of the system. In other words, they are never tested for correctness.
How many times have you come across a comment that appears to have no bearing on what the code is doing? In heavily commented code, this happens all the time because a developer (not necessarily the original developer) makes a change and does not update the comment. It could be that the developer was in a hurry and sloppy, or that the change was upstream and made the comment irrelevant, or maybe the code was copied and pasted into a context that doesn't match the comment, or whatever. The point is that comments can become stale, and a stale comment is worse than no comment at all.
Which is why I don't buy what the article tries to sell, at least not fully.
In the first example, having a few initial handle-and-exit if statements to deal with edge cases, and a main body that's clean means you don't need to comment.
The approach suggested by some here that you could just have two for loops over the input, while clever, might not be immediately obvious to everyone and as such would probably need a comment explaining it.
The latter does not need special case handling, but is less obvious.
I consider this behavior to be a sign of an immature dev. If I ever see a senior engineer do this, I'll know he/she's probably over-leveled.
He’s definitely junior in this case. He’s only in his first programming job out of college. I tried to get him to see the error of his ways but not everyone will listen to reason. :)
CSS for example might assign a button to be green. Should you trace back to the specific requirement to say why it’s green or can you trust if somone changes it to blue it’s becase the requirement changed. I would generally say the second.
Ideally, the vast majority of requirements can be treated as such. Cases where I have wanted to include the comments generally relates to brittle code where some change likely has knock on effects. Ideally such code should be avoided where possible. IMO, such things are also better cought in unit tests and documented in source control providing more context.
That said, including things like design goal can be helpful to get people familiar with the system. But again IMO, specific requirements should rarely sit in comments rather than unit tests, design documents etc.
Exactly, and I don’t think a comment about an old requirement would generally do so. Someone just gave you the new requirement which presumably replaces the old one.
But, if the comment mentions 508 usability as an issue or as you say it’s shaped like a camel, then that’s going to be an issue for any version of the website you create. Basicly, comments that make it on the minimum list are suck there and don’t become relevant when comparing different possible designs.
Code smell is about comparing designs and implementations not high level requirements. If 1/2 your customers uses JAWS then you got to do what you got to do.
It’s literally hiding your intentions and reasoning for doing things, which I think is anathema to good code maintenance.
Then it is natural to iterate over the input and not the output:
for (int i=1; i < length; i++)
output[i-1] += input[i];
for (int i=0; i + 1 < length; i++)
output[i+1] += input[i];
On an unrelated note, why do you use i+1<length rather than i<length-1 ?
You want to search an array for an element:
int i;
for (i=0; i < array.length; i++) {
if (array[i] == value) break;
}
return i;
array.push(value);
int i;
for (i=0; ; i++) {
if (array[i] == value) break;
}
array.pop();
return i;
An example of this in action is LLVM's MemoryBuffer [1], which is input to a parser. How to write a parser? Perhaps it performs bounds checking at each production. However LLVM guarantees that the MemoryBuffer is NUL (0) terminated. A parser can then arrange for NUL to be a terminating character, and handle it uniformly.
The resulting parsers are faster, and also significantly clearer and easier to write, because there's no need for bounds checking at all.
For a parser that's probably not a problem (it can transform NULs in the input to a different token), in other situations it's not so easy.
Clever example of this approach, though!
So that means some small amount of the time, a linear search (a supposed read-only op) results in a possibly massive allocation or I need to allocate one extra space each time? Search is now not multi-reader thread-safe either. I mean, I can see the value, but surely a modern filter findAny is superior to this.
EDIT: can't post in response but I thought you meant that this trick carried over.
Trying to expand the Array by prepending and appending to it shows lack of knowledge of memory, performance, and maintainability. If loop is refactored away from 'padded', all hell breaks loose 3 months down the line.
A good example of this is making values non-nullable by default, with Optional types, and eliminating a huge swath of special case handling.
But the benefit doesn't show up without significant scale, and isn't worth it for functions that are just used once or twice.
I studied Maths at uni so I definitely have a bias to the author's position of transforming the problem into a form where an elegant solution "falls out" naturally. However, I think this is because I am comfortable (from training) with transformation steps and can easily see whether they matter or not in terms of affecting the answer.
The example the author uses seems nice because their entire audience is experienced/intelligence enough to see why padding zeroes works.
There's a puzzle where you are given an array of ints and told every term appears twice except one. You have to find this "lonely element". This can be solved naively by looping through the list and recording how many times you see each number.
It can also be solved "elegantly" by XORing the whole list - but it is not immediately obvious why this works. In this case I would definitely say the elegance is not worth the obfuscation cost.
The personal compromise I've come to is to try and refactor the code so that specifical cases are extracted and quarantined as much as possible, leaving the main algorithm hopefully clear and elegant.
For example, in the article's first example, I would not pad the array but would split out a separate function neighbours(i) which returns [1], [n-1] or [n-1, n+1].
This approach makes more sense in more complicated examples, (say a 3D grid) or in particular if padding is impossible, like you have to calculate (sum of neighbours) + (product of neighbours).
I'd say this is a special case (ha!) of the principle that each person and organization has their own personal tendencies. If you say "X is a code smell" and the internet disagrees, it probably means you or your organization have a tendency to choose X when it's not the right solution.
Basically he meant, if the problem isn't presented in a way that makes it convenient to solve, rearrange the problem fist, then solve it.
A classic example of this from high school math would be: https://en.wikipedia.org/wiki/Completing_the_square
The way I commonly approach edge cases is that they are dealt with by an additional rule (and possibly accompanying data structure) in otherwise existing similar common functionality. If current functionality is not sufficient then write new functionality to solve for and replace other existing insufficient functionality. This allows for consideration of edge cases as formal requirements while minimizing expansion of complexity. It is refactoring and not innovation.
If there is a need for innovation and new functionality then the requirement is a dedicated feature instead of an edge case. The difference between a feature and an edge case is the different level of intentional overhead required for documentation, integration, testing, and maintenance.
Repeated below:
At the opposite end of the spectrum, I actually wish more people understood the really core low-level kind of coding. Not big, complex stuff like the lockless name lookup, but simply good use of pointers-to-pointers etc. For example, I've seen too many people who delete a singly-linked list entry by keeping track of the "prev" entry, and then to delete the entry, doing something like
if (prev) prev->next = entry->next; else list_head = entry->next;
and whenever I see code like that, I just go "This person doesn't understand pointers". And it's sadly quite common.
People who understand pointers just use a "pointer to the entry pointer", and initialize that with the address of the list_head. And then as they traverse the list, they can remove the entry without using any conditionals, by just doing a "*pp = entry->next".
12pm is usually used for noon. Ironically, of the two choices, it makes the least sense. It makes the PM hours become:
12, 1, 2, 3...
When one would expect:
...9, 10, 11, 12
Unfortunately, people weren't quite as into zero-indexing in ancient history.
00:00 is not ambiguous, though. There is no such date on 12-hour clocks (there's no zero! Just 12…), and on 24-hour clocks it's unambiguously the beginning of the day (24-hour clocks are zero-indexed).
In fact, 00:* dates are consistently unambiguous, because they don't require you to define whether you're using a 12 or 24 hour clock. I think you're thinking about 12:00 on 12 hour clocks.
In any significantly complex system of business logic, special cases abound. A system of only special cases is of course bad, but a system with a few here and there are expected.
$input = array(1, 3, 4, 5, 0, 1, 8);
$i = 0;
echo $input[$i - 1] + $input[$i + 1];
// outputs 3Write it out as straightforwardly as possible. Add some unit tests for the special cases. And then move on to something more worthy of your time than trying eliminate a few if statements.
It reminds me of programming assignments where you're tasked to solve a maze. Exploring the maze is really annoying if you have to special case the edges; much easier if you just build an edge-of-the-world wall first.
> Notice how we treat the endpoints as special cases, complicating the simple rule “sum both neighbors of every element.” A better approach is to first transform the input so the special cases vanish, leaving only the general case.
Why is that a better approach? The author never deigns to share their reasoning.
Write for maintainability and extensibility. I know you can solve the given problem better, with more succinct syntax and clever shortcuts. Please don't though. The code you write today is here to stay. Plan for the war, and don't overoptimize the individual battles.
Padding array makes you go "wtf is that?"
I would rather see something like this: (i > 0 ? input[i-1] : 0) + (i < input.size - 1 ? input[i+1] : 0) which clearly maps to "if there is a left neighbor, add it; if there is a right neighbor, add it; return the sum."
Business is smelly, messy, and just filled with exceptions, special cases, and weird deals with an diabolical structures.
Most startups start with a beautiful, elegant system with zero real users, then they hit traction, and with added business the business logic parts of the code base turn into eldritch monstrosities.
If you say:
Oh by the way, if you want to call "final_step" be sure to remember to mirror your input array otherwise my method will break.
I would say:
Oh by the way, You are fired.
instead of "result = (1...padded.size-1).map do |i|" shouldn't be "result = (1...padded.size-2).map do |i|"?
It's always felt deeply counter-intuitive to me because .. feels like it represents less than ... but that's a separate discussion.
Thus the ultimate way to organize a program is to have a clear separation between the specific and the general.
var sum = 0;
for(v : vals) {sum += 2*v;}
sum -= (vals.first + vals.last);
(Proof: when you sum the neighbours of all the elements then you add each element twice except the first and the last one.)
Even simpler and even faster:
var s = sum(vals) * 2 - vals.first - vals.last; result = [0] * len(xs)
for i in range(0, len(xs) - 1):
result[i] += xs[i + 1]
result[i + 1] += xs[i]
return resultGet used to it, kid.