Quite often when I'm pouring through an old codebase or evaluating a new codebase, I run into a variety of things that drive me nuts.
First, there are quite often duplicated/similar function names like doSomething(), doSomething1(), and doSomething2(). Every single one does something very similar to all the others but normally there's a special case handled in each one… or more annoyingly, a not-so-special (aka normal) case. As confusing and annoying as those sound, they're not the worst.
Next, there are often functions that are basically big case statements that perform completely different behaviors based on a single parameter. As wasteful as those often are, they're still not the worst. (For reference, this is often called Conditional Complexity or Combinatorial Explosion.)
In my opinion, the most annoying and worst for cleaning up the system are the functions/methods that make too many assumptions for you. It could be a variety of things – assuming a database connection, assuming what the output destination is, or assuming what the input is – but I most often see it in scrubbing/cleaning functions. A trivial example:
function cleanString($input) {
echo strip_tags($input);
}
What assumptions does this function make?
First, it assumes you want to do a strip_tags. While this often makes sense for numerous reasons, there's no way to override this function and provide a set of allowed tags. If this is a function in our own code, no problem, we can add a second optional parameter and roll on. If this is deep within a supporting library, then we can't do that. We either have to accept the limitation or roll our own function to do something very similar but not quite.
Second – and this one is 10x worse in my book – it assumes that you want to immediately echo the result. That's funny… the name doesn't say that. The name of the function is “cleanString” not “cleanStringAndEcho“. Now, if we needed the same functionality for inserting into a database, we have to duplicate code. If we need to do the same thing for an email, we have to duplicate code. If we want some sort of formatting, styling, etc, we have to wrap this code in something else.
This could be easily resolved by simply returning the result. Then the calling code could determine what should be done with it.
Unfortunately, in terms of offending code, this stuff isn't the worse… this is just the appetizer.
There are other bits of code – normally supporting libraries – that behave even worse. The one annoying me at the moment is a database abstraction layer. When it hits an error – like incorrect credentials – it simply stops and delivers an incomprehensible error message to the user. While I can't prevent the error message from displaying, I can catch the unstable state and stop further execution.
With the exception of major operations* which could cause irreparable harm, supporting libraries should never stop my application. That is rude at minimum and infuriating and detrimental to me at worst.
* If you're not sure what “major operations” might necessitate such an action, talk to your team members, boss, and customers.
If you're building a library or set of libraries you want other developers to use, here are some tips:
Do not stop my application.
Do not deliver incomprehensible error messages to my users.
Do not prevent me from handling the error when (or even if) I choose.
Do not give my users a bad experience, that's my job let me recover gracefully.