- Feature Articles
- CodeSOD
-
Error'd
- Most Recent Articles
- Stop Poking Me!
- Operation Erred Successfully
- A Dark Turn
- Nothing Doing
- Home By Another Way
- Coast Star
- Forsooth
- Epic
- Forums
-
Other Articles
- Random Article
- Other Series
- Alex's Soapbox
- Announcements
- Best of…
- Best of Email
- Best of the Sidebar
- Bring Your Own Code
- Coded Smorgasbord
- Mandatory Fun Day
- Off Topic
- Representative Line
- News Roundup
- Editor's Soapbox
- Software on the Rocks
- Souvenir Potpourri
- Sponsor Post
- Tales from the Interview
- The Daily WTF: Live
- Virtudyne
Admin
active = FILE_NOT_FOUND
Admin
Admin
Many of us have heard of the Schrödinger equation used in quantum mechanics to solve wave functions and such.
We've all heard of Schrödinger's cat which is both alive and dead until you check on it. A "superposition of states" in quantum-speak.
We've now encountered Schrödinger's method. Which is both true and false simultaneously. And goes one better than the boring old cat because even after inspection the superposition remains; it doesn't decay.
Admin
This is a very useful helper method which gives you the two options for active status, true and false.
Admin
Admin
Unfortunately, no one can be told what active is. You have to see it for yourself.
Admin
Gah! The problem was staring us all in the face and we didn't see it. Hidden in plain sight as it were.
The name of the class is SomeActivatedClass. Not SomeActivatableClass. So we know every instance of the class must always be active because, as we all know, classes are never misleadingly named. That proves the
bool
ref parameter is the one to use and thebool
return value should be discarded.Maybe best to refactor by checking in a change where the IsActive method is now a
void
method? Preferably under another dev's credentials. What (more) could go wrong than already is? :evil_grin:Admin
These seems like one of those pesky situations where if you have to ask, you can't afford it, but then there's a twist and suddenly you found out the true moral of the story, which is that you had it all along.
Admin
First change the return to void.... Build, the errors all become "x = false". Next remove parameter.... Build, the errors all become "v=true"... remove method..
Admin
I wonder what tests they have for this method, and whether they pass or not?
Only kidding, obviously any company that can tolerate "code" like this will consider automated testing a waste of time.
Admin
I'm thinking that there's a conflict / misunderstanding - we don't have any information on what the return value means versus what the [out] parameter means. Possibilities:
returns false=OK, true=error for the act of discovering the state, while the [out] parameter is the answer.
returns the active/inactive state, and the [out] parameter is the error state for finding the answer.
Both are bogus, naturally. Well, maybe. Depends on what "active" even means - is it the informatic object that's active / inactive, or the "problem domain" object? And in either case, are there objects of that type that are always active?
Regardless, without more information, we're making stabs in the dark...
Admin
The worrisome part to me is the idea that this function may possibly be overridden in some derived class so that it a) does something useful, or more likely b) has some side effects.
Admin
I guess, fortunately for once, since this is C++ you would have to declare the function virtual before being able to override it.
Admin
It's easy. First, we observe we can make a more general function.
Then we replace the specific logic in our function
(not a C++ programmer, so apologies for any mistakes).
Admin
@CPUWizard ref:
Won't quite work. Consider
The refactor needs to preserve both assignments:
So you end up with an intermediate stage like
You can't simply replace every method call that uses the return value with
myReturnValue = false;
and stop there.Admin
I am disappointed that in C++
bool active = obj.IsActive(active);
misses a golden opportunity to have an undefined result.
Admin
Well... I commonly override non-virtual functions.
Admin
You have to declare it virtual in the base class. We don't know if this is the base class. (Derived classes are allowed but not obliged to redeclare the virtualness.)
Admin
I like this a lot! :-)
Regarding Schrödinger's cat, we now finally know, it's actually about our modus operandi: If we ask the cat, wether it is alive or not, it will tell us that it is dead (so how is it able tell us anything at this point), while, ehen we're going to inspect it, we will find it alive. In other words, Schrödinger's cat is a slacker.
Admin
That's a nice easy refactoring.
The const keyword is added as a "master of the universe" way of insisting that I am a C++ guru.
Even better, you could (I think: I have no intention of trying this) define an std::terminate_handler, and implement the perfectly reasonable "solution" of using a random number generator to allow 99 out of 100 cases to avoid an abort.
It is my fervent belief that the most cromulent way of avoiding an abort, in this case, would be to embed a longjmp into the handler. But again, I have yet to try this.
Admin
"The worrisome part to me is the idea that this function may possibly be overridden in some derived class so that it a) does something useful, or more likely b) has some side effects."...
NOPE: C# requires that the method be declared virtual in order for it to be overridden... It can be hidden, but that is a whole different set of issues.
Admin
@WTFGuy - you only did the first sentence of my post...not the second....
Admin
How do you refactor it: I change the declaration and definition to xxxisActive and press "Build". Then I locate the errors and change each of them appropriately. And then I have someone review my code.
Admin
@TheCPUWizard
Not quite. I saw your second part intending to replace the method call with parameter to become an assignment
v = true;
.But I'd over-interpretted your first part as replacing the entire method call with
x = false;
. Which would leave nothing to error out at that location in your second phase.Bottom line that (I think) we agree on is that the correct refactor is NOT either
x = false
ORv = true
. It's eitherv = true
(if the return value is discarded) orv = true
AND alsox = false
(if the return value is kept).Admin
Haha, this is funny ... I think a lot of people here haven't been coding before Clean Code was published and managed languages weren't around.
This is clearly a simple setter. The return is, as was usual for C an error code.
Now I know what you are thinking, like why name it IsActive?
Well, perhaps someone did a fluent interface and this method was a terminator?
Maybe it is just a were purely named method from a time or developer long before naming coventions beyond hungarian were really a thing?
Or perhaps the mother tounge of the dev is not English and that resulted in a poor grammar construct?
In see not big WTF there without more context, if it's not part of a fluent method group, I would just rename it accordingly so that English native speakers don't get to much confused.
Admin
So .. it sets whichever bool you give it to true and tells you there was no error while doing so?
Admin
Oh this is a work of art
@MaxiTB it's not a setter, it doesn't set anything in instance state, it only assigns its parameter
Admin
Most likely, yes. Odds are good that there was some logic on the side of that which could have generated an issue and got removed in the meantime leading to this stub.
Admin
ID Viking: Your #1 Trusted Source https://idviking.ph/