G'day
I'm going to pass this on to the CFWheels Dev Team, but it's applicable in anyone's code, so posting it here first.
A lot of the code in the CFWheels codebase hasn't really been "designed". It's been written as if the developer concerned just went "OK, I have this file open… I'll start typing code in this file…", rather than giving thought to the notions of design patterns or the SOLID principles and stuff like that that exist to keep codebases… usable.
It's legacy code, and we have all written code like this in the past, so let's not dwell too much on how the codebase got to where it is now. Let's just accept that it could be better than it is.
But It's 2022 now, and there's no real excuse for perpetuating coding practices like this.
An example in front of me ATM is the onApplicationStart method. It is 1000 lines long. That is ludicrous.
The design problem in this particular method (and this is the same for a lot of code in the CFWheels codebase) is that it confuses "the things I need to get done", with "how to do those things". It's onApplicationStart's job to… start the application. There are numerous steps to this, but instead of it just calling all the steps it needs to call to define "starting the application", it also has all the implementation of the steps inline in it as well. This is an anti-pattern.
One can even see what each of the steps are. There are comments identifying them. In fact the first two are done "right":
// Abort if called from incorrect file.
$abortInvalidRequest();
// Setup the CFWheels storage struct for the current request.
$initializeRequestScope();
(Except the comments here are pointless as they simply repeat what the method names already say clearly).
But then the wheels (sic) fall off:
if (StructKeyExists(application, "wheels")) {
// Set or reset all settings but make sure to pass along the reload password between forced reloads with "reload=x".
if (StructKeyExists(application.wheels, "reloadPassword")) {
local.oldReloadPassword = application.wheels.reloadPassword;
}
// Check old environment for environment switch
if (StructKeyExists(application.wheels, "allowEnvironmentSwitchViaUrl")) {
local.allowEnvironmentSwitchViaUrl = application.wheels.allowEnvironmentSwitchViaUrl;
local.oldEnvironment = application.wheels.environment;
}
}
application.$wheels = {};
if (StructKeyExists(local, "oldReloadPassword")) {
application.$wheels.reloadPassword = local.oldReloadPassword;
}
That could should not be inline. There should be a method call inline, called something like configureReloadOptions (or whatever this is doing… a case in point is that I can't really tell).
The thing is onApplicationStart should not be defining the implementation of this, something else should be doing that, and onApplicationStart should just know how to call it.
Why is this an issue?
Because for us, one of the sections of processing in onApplicationStart is wrong, and sufficiently wrong it's been flagged up as a pen vector.
If each part of the application-start-up sequence was in its own functions, then we could override the function with our own implementation to address the issue. And the change would be local to that one small function, so the testing surface area would be small. But instead I now have to hack a third-party codebase to solve this. It's 100s of lines into this method, and accordingly its test surface area is huge (and largely unreachable, I think).
This is basic Open/Closed Principle stuff. The O in SOLID.
My recommendation / rule of thumb is that whenever one is tempted to put a comment in that explains a block of code (or delimit it from other blocks), then that block of code should be in its own function. If one follows this rule of thumb, one seldom gets into the situation that one has Open/Closed Principle issues. It's not foolproof, but it's a good start.
Template Method design pattern
2023-01-31
As Ben points out in the comments, this is an example of the Template Method design pattern at work. So it's not like I am advocating some esoteric approach that I made up myself, it's an established design pattern we ought to be using.
Cheers for the heads-up Ben.
Also having a code design rule that functions should start to raise a flag if they are over 20 lines long. They are probably doing too much, or aren't factored well. If they are more than 50 lines? Stop. Recode them.
If a function has unrelated flow-control blocks in them? Also a red flag. Each block is likely more appropriate to be in their own functions.
A function like onApplicationStart that needs to carry-out a _lot_ of steps to consider the app to be "started" should look something like this:
function onApplicationStart() {
doTheThing()
doAnotherThing()
doSomethingElse()
doThisThingRelatingToStuff()
doOtherStuff()
doThis()
doThat()
// ...
}
It really shouldn't be implementing any stuff or things or this or that itself.
Lastly, someone drew my attention to this statement on the CFWheels home page:
Good Organization
Stop thinking about how to organize your code and deal with your business specific problems instead.
This possibly explains a lot of the way the CFWheels codebase got to be the way it is. It is really bad advice. One should always be thinking about how one's application ought to be designed. It's important.
NB: this is not just related to onApplicationStart. That's just what currently has me sunk. It relates to really a lot of the code in the codebase.
This is also something that can be tackled in a piecemeal fashion. If one has to maintain some code in a really long method: extract the block of code in question into its own function, and maintain (and test that).
Righto.
--
Adam
