Notice that x()and y()are not constructors.
Now, a "good" user of an instance of Trouble will do something like this:
t.x(); // always call x() before y()
t.y();Whereas a naive user of the class might do something more like this:
t.y();
In this case the value of i1 (used by function y) is either undefined or possibly some leftover value from earlier uses of instance t.
How Do You Know?
The functions x and y have an implicit temporal binding. When you look at the object diagram, or if you use code completion, nothing you see will tell you that you must call x before y.
Either:
- You don't know about it, and have been "just lucky" so far
- You don't know, and are currently making bugs you don't know about
- You know because you've read the code from top to bottom and understand the implicit temporal coupling.
- You copied someone else's example after yours didn't work, and you don't know why it calls x() before y(), but dammit it works.
- You've spent considerable time in trial-and-error and treat x() as a magic incantation.
- Someone told you.
Of these, I'm most afraid of 1 and 2. These are hidden dangers, and eventually there is going to be a nasty surprise for someone -- possibly the customers.
If one is working at a large scale (dozens or hundreds of people using Trouble) then 3 is asking far too much. Who can afford to carefully read the implementations of all the classes they use, and keep track of when variables are set and used? You can't afford to have 20 people wasting time on this, in hopes that they might spot the one or two special classes who rely on temporal coupling.
While it's easy to blame all "bad code" on a lack of discipline, we find that it is more productive to fix the tricky coding practices that require a high level of discipline.
You will always find it more scalable and productive to make the work easier than to work harder.
Don't work people harder.
Make people's work easier!
Number 4 is also a nightmare at scale. Code duplication is the king of code smells, but here people are copying code because they feel like they can't afford the time to understand the code they're producing. That's awful. I don't blame the copiers, they're in a situation where they're asked to put out effort they can't afford. But I would rather they investigated instead of copying blindly.
Number 5 is too much to ask.
Number 6 (someone told you) is not bad in and of itself. Communication is good. But here the communication is how to work around a problem in the code so that you don't have to fix it. Perfectly reasonable-sounding if you can't change the code itself, but questionable in general. How safe are you if the only thing protecting you from disaster (in the hazards of 1 & 2, or the time sinks of 4 & 5) is oral tradition.
At Scale?
While solutions often scale poorly, we find that problems always scale very well, indeed.
The ideal when programming in groups and at scale is that you demand the least from each other, so that you can all accomplish the most.
At scale, you want to build code so it is easy for other people to do what works well, and avoid hazards and risks along the way.
All implicit couplings are risky, including (but not limited to) duplication of algorithms. Every fact, every algorithm, every bit of knowledge should have a Single Point of Truth in the application.
Here the design of the code is such that the "point of truth" (knowledge that x() must be called before y()) is distributed to every bit of code which needs to call function y.
It demands duplication of code in the callers, and demands a higher level of "due diligence" from the programmers.
At scale, this small feature becomes a big problem.
So now what?
There usually ways to fix this problem. There are also ways to cope with not solving it.
- Make y call x.
- Make incorporate y into x, if y really only means "complete work started in x."
- Have y set i1 directly.
- Initialize i1 to a null or indicator value in the constructor, and add a guard clause in y which will throw an exception if i1 has not been given a meaningful value.
- Create a new function that calls x() and then y() -- and rename function y to something that sounds dangerous to use, like unsafeY or innerYtoBeUsedOnlyIfI1HasBeenSet.
- Rename x to prepareForY.
- If only x and y use i1, then it is a temporary field (code smell) and you should refactor it away normally.
- Add an optional parameter to y, which is the value you want i1 to have. Combine this strategy with the guard clause idea, above.
- Build a wrapper class around the Trouble class, so that the wrapper is the Single Point Of Truth about how the Trouble class should be handled. Make the Trouble class private, internal, or otherwise hidden.
- Go find or make a replacement class for Trouble. Who needs it?
- Tolerate Trouble by setting a cron job to search for invocations of y. Review any new uses of the function immediately.
You can probably come up with a dozen more ways to improve the situation.
But the important thing is that we can recognize implicit temporal cohesion and its role in making code expensive to write, and that we take steps to make the work easier if we want to accomplish more work sooner.
And who doesn't?
No comments:
Post a Comment