Read on Twitter
Me, at every other line in this legacy codebase that should have never been written.
I've been dealing with legacy code for 17 years. I have developed a thick skin for some of the worst code. I was *still* not prepared to see something like that.
There are pages and pages of assignments using nested ternary operators, which get data from stdClasses where every property is potentially non-existent. The whole thing looks so dense that I feel despair when something crashes in there.
There are hundreds of lines of unnecessary date parsing that is actually error-prone, causing one to occasionally split booleans into an array, then access non-existing array elements, then persist the broken result back to the database, thanks to loose types and disabled errors.
A base class that is 4000 lines. One can pass a dozen arguments to its methods. Most of its properties are assigned dynamically. If I were to extract them, I'd end up with several pages of nullable properties, which of course can cause tons of temporal errors.
Ultra-fat controllers. Entire folders of abandoned code. Too much code duplication and tons of duplicated bugs as a result. Calling a controller repeatedly for every element in a combo box. Every other line is a new surprise.
Most type hints are missing and the ones there are often lying, making it that much harder to debug.
The obviously unreachable execution paths represent thousands of lines of code. The control flow and the way methods are split are completely inadequate, forcing you to pretty much throw exceptions if you want to get out.
Overriding properties with objects of a completely different type at the beginning of various methods, for the purpose of another method to eventually call something on that object. Unless you put a breakpoint, you can't know what object the property contains at any given time.
Complete disregard for the HTTP protocol, making destructive actions on a GET.
Overuse of sessions, global variables. Liberal use of God classes.
Massive HTML blocks inside of controllers and massive PHP blocks inside of views, going as far as instantiating repositories in there.
exit; statements in too many places.
A form that sends a csv value to the backend, which then assumes that it is a csv without any validation, then concatenates it into a WHERE IN clause.
There also an eval that instantiates the classes used for input validation.
A global function inside a view instead of a view helper... copy-pasted into each view that needs it.
if (SomeClass::NEW_LAYOUT) {
// 3 lines of code
} else {
// 200 lines of dead code,
// leading to another thousand lines of dead code in an abandoned module
}
Note that NEW_LAYOUT is a constant.
// 3 lines of code
} else {
// 200 lines of dead code,
// leading to another thousand lines of dead code in an abandoned module
}
Note that NEW_LAYOUT is a constant.
Using a variable to control how many results per page should be displayed, while the repository pulls a global. This causes the results to be out of sync with pagination links.
$this->check($query, 1);
public function check($q, $check = '')
{
if ($check != 1) $q->where('...');
if ($check != 2) $q->where('...');
// etc.
}
I suppose that these people write the code once and hope to never have to read it again.
public function check($q, $check = '')
{
if ($check != 1) $q->where('...');
if ($check != 2) $q->where('...');
// etc.
}
I suppose that these people write the code once and hope to never have to read it again.
Worthless comments that just repeat the name of the method, but with spaces.
Too lazy to figure out the inverse of a boolean expression, so just using the else block, while leaving the if block empty:
if ($expression) {
}
else {
// do something here
}
if ($expression) {
}
else {
// do something here
}
The exact same errors copy-pasted across 14 modules. Over a dozen of such errors.
Extremely ambiguous method names, such as "import" in a context where there are 11 types of data import. Variable names often have absolutely nothing to do with the value being assigned to them.
Once unused variables are removed, dozens of other lines also becomes obsolete, often leading to the removal of several execution paths.
Methods that return things like "string|string[]|null" are rampant, leading to all sorts of bugs silently waiting for the right user input to blow up in your face.
A time of "00:00:00" actually means no time, so it's impossible to set anything to exactly midnight in the system.
Tons of echo/print in methods, instead of returning the string and letting the caller decide what to do with it. Also often assigning the output to a local protected property, which may or may not be null at the time of reading from it.
die('Some Message');
echo '<pre>';
print_r($someVar);
exit;
$this->repo->saveSomeStuff($stuff);
exit;
All of that still accessible via the web.
echo '<pre>';
print_r($someVar);
exit;
$this->repo->saveSomeStuff($stuff);
exit;
All of that still accessible via the web.
Dead PHP code commented out in views. But it's commented out using <!--, so the PHP still runs and is actually causing the views to sometimes explode due to undefined variables and bad array keys.
Many type hints with classes that don't even exist.
Else blocks that reference undefined variables. Clearly, many paths were never tested, even manually in the browser.
Nested foreach loops sharing variable names. In PHP, this leads to all sorts of nasty bugs.
$size = file_get_contents($filename);
if ($size) {
$handle = fopen($filename, 'r');
// process the file
}
Note that file_get_contents read the entire file, it doesn't give you its size. This code actually explodes, because the file is too large to fit into memory.
if ($size) {
$handle = fopen($filename, 'r');
// process the file
}
Note that file_get_contents read the entire file, it doesn't give you its size. This code actually explodes, because the file is too large to fit into memory.
A whole bunch of
$firstName ? $firstName : '';
that aren't assigned to anything or used in any way. They're just there for no reason at all.
$firstName ? $firstName : '';
that aren't assigned to anything or used in any way. They're just there for no reason at all.
Big chunk of code repeated inside the same method, about 30 lines lower.
N+1 problems all over the place, causing some pages to take ages to respond.
Calling view partials and helpers inside the controllers in order to stick the resulting HTML into a JSON.
Calling a method that takes zero arguments with one argument, possibly one that existed long ago. This also warranted the removal of 42 lines that each created their own copy of this argument... in one long method.
More than half of the execution paths that should return a value do not have a return statement. This means that there are nulls in places that don't expect them, and any deviation from the input format can cause a NPE (null pointer exception).
Lots and lots of method parameters that are not used in the method, but need to be passed anyway for this mess to not crash. Can't auto-refactor either because of evals in the code, which prohibit my IDE from accurately finding all usages.
Tens of thousands of null pointer exceptions. Sometimes more than 10 in a single statement.
SELECT query results assigned to unused variables, making the whole query pointless.
return true;
//This code is not using now
Followed by a ton of unreachable code that was left there to rot.
//This code is not using now
Followed by a ton of unreachable code that was left there to rot.
Dead code copy-pasted across hundreds of files.
Attempts at assigning properties of non-objects. Example:
$list[$key]->propertyName = $value;
Where the item might actually be an array instead of an object.
$list[$key]->propertyName = $value;
Where the item might actually be an array instead of an object.
All endpoints are available using any HTTP method. Some endpoints, if accessed using GET, will crash with a fatal error in production. The HTML sent until the crash point will be visible in the browser, with the CSS missing.
It's not just the code quality that hurts. Even the UI leaves me confused. There is a whole screen worth of content between the search results and the button that allows you to go to the next page of results.
A controller that relies on something having been put in the session 3 pages earlier. It explodes if the thing is not in the session.
Assigning a local variable to null immediately before returning from the method.
A single, critical line of code commented out, making the entire class no longer have any purpose, making it dead code. The class prepares tons of data using complex logic just to not send it anywhere.
A variable named "with" that has absolutely zero meaning within the context where it is used. IF ($with) { // send this e-mail } else { // send this other e-mail }.
Within a controller, 700 lines of densely concatenated HTML with inline styles. And of course there's a style bug in there somewhere.
A switch/case that spans 650 lines. 62% of the cases are unreachable, because the string that they check against used to be set in a module that has long since been discontinued. Removing them also obsoletes another 400 lines.
A large IF block that contains the exact same content as its ELSE counterpart.
In the same controller, two methods of 132 and 181 lines. They only have one difference: the second method appends some extra content to a string, even though both methods should have it, making the shorter method redundant.
Checking whether an array is empty, to then iterate over it, in order to return the value matching a specified key, instead of just `return $array[$key]`. Optionally, one can use `$array[$key] ?? $default` to return a value in case a key is not found.
Passing 4 arrays and an integer to a function. The function returns one of the 4 arrays based on the integer.
function ($id, $list) {
$itemId = $id;
$itemList = $list;
//...
}
Instead of just:
function ($itemId, $itemList) {
//...
}
$itemId = $id;
$itemList = $list;
//...
}
Instead of just:
function ($itemId, $itemList) {
//...
}
Hundreds upon hundreds of repository methods, mostly permutations and often used only once. Yet there are still countless SQL queries inside of controllers and non-repository classes. Any minor database change would wreak havoc on this codebase.
