Comment On How Did No One Think of this Before?

I think I just wept blood!

Why do people do these things?

I really hope the author planned his own confirmation box with some fancy eye-candy or anything else.

But the if-statement is really stupid, mwahahaha

A bit boring, but I do things like this all the time due to bad legacy-code!
What if the confirmMessage() is called by old code you don't want to touch and you then find the new and hip confirm() and just use confirmMessage as an adapter....

But still, I would use return confirm('Message') though...

/P, cheating in C#

Not a WTF.


I've seen this hundreds of times in two different cases:

1) confirmMessage used to have more logic but was refactored out, however the code calling this function is too numerous to refactor as well.

2) confirmMessage will eventually become more complex, but confirm is used in other places that don't need the future logic, so this placeholder is created so that other modules can begin to use the function even though it is not complete yet.


Therefore, not a WTF.

Why not?:

A bit boring, but I do things like this all the time due to bad legacy-code!
What if the confirmMessage() is called by old code you don't want to touch and you then find the new and hip confirm() and just use confirmMessage as an adapter....

But still, I would use return confirm('Message') though...

/P, cheating in C#

confirmMessage = confirm;

Yes, you can do that in javascript. And if you're planning a "fancy confirm box", you can override confirm itself the same way [however, it is a little-known fact that it is IMPOSSIBLE to do a 'fancy confirm box' - the only functions that can block execution in javascript are alert, confirm, and prompt. Of those, only alert can reasonably replaced with a non-blocking function.]

In this case the code could have been factored out. Still it could return just the result from confirm, but perhaps they want to use that to perhaps call a logger later on or whatever...

I have seen such stuff also very often.

Aye, this is just a way to abstract away message displaying. This way it's easy to change how a confirm dialog is displayed (be it the normal confirm() box, or some popup or similar (not that that wouldn't be a WTF)) without altering all the code that uses confirmation dialogs.

but how else can we get such flexibility in our code?

Imagine the sibling functions that exist beside this one:

ConfirmTextData(TextData) /For text coming and going to a dataset
ConfirmString(string) /For non-message strings

The possibilities are endless!

But I agree, this was simply bad code refactored out in a manner that prevents the need to change all other code. Essentially a fix for a much worse WTF.

We have a system that uses something like this, but 2/3 of it is owned by another group. As such, refactoring stuff like this is hard as it requires inter-department coordination of releases. As was mentioned earlier, sometimes, it's just easier to wrap someone else's garbage. Although, "return confirm(msg)" would have been a better implementation.

If 'confirm' is in a third-party library, COM object, etc. for which source code is not available, then this wrapper function will give you a convenient place to set a breakpoint, or add debugging or logging code, especially when there can be hundreds or thousands of calls to it throughout a system.

Because one day you might want to use DHTML confirmation windows...

The horrible truth is that pointless, obfuscating layers of abstraction are common as dirt in any large OO codebase.

There are multiple reasons: laziness or fear of refactoring, incomplete understanding of OO design, or (probably the worst) a fresh love affair with the GoF book (a phase that many programmers seem to go through).

Random832:

Yes, you can do that in javascript. And if you're planning a "fancy confirm box", you can override confirm itself the same way [however, it is a little-known fact that it is IMPOSSIBLE to do a 'fancy confirm box' - the only functions that can block execution in javascript are alert, confirm, and prompt. Of those, only alert can reasonably replaced with a non-blocking function.]

It's not impossible to achieve that effect, though. You can block off the entire page by positioning certain elements over the entire window, and then pop up your HTML based dialog box in the middle. And maybe your confirm dialog doesn't need to be modal? There are reasonable justifications for wanting a non-modal dialog box, and this provides a way to achieve those results.

I believe (it's been a while since I've tried to, and IE was the only platform being used) that it is also possible with browser specific code to create your own modal popup. I'd hope nobody would choose that route on a public-facing website, however.

I agree. However, how about a little comment that would justify the redundancy of the function?

W:

Aye, this is just a way to abstract away message displaying. This way it's easy to change how a confirm dialog is displayed (be it the normal confirm() box, or some popup or similar (not that that wouldn't be a WTF)) without altering all the code that uses confirmation dialogs.

Exactly. If it's a method used in 200 places it's easier to make a change once than to make it 200 times through the whole codebase.

I'm dealing with a lot of similar abstraction garbage. The worst part is that this is brand new code written in the past 4 months. The reasoning is that we *might* want to override those functions at some point in the future...

If it were simply a case of encapsulation the code would say
function confirmMessage(msg)
{
return confirm(msg);
}

Nope, this is defitely the work of a twit and was not intentional.

function confirmMessage(msg)
{
if ( confirm(msg) )
return true;
else if ( isset(fnf_flag) )
return file_not_found;
else
return false;
}

Just upgrade-friendly code...

ydant:

Random832:

Yes, you can do that in javascript. And if you're planning a "fancy confirm box", you can override confirm itself the same way [however, it is a little-known fact that it is IMPOSSIBLE to do a 'fancy confirm box' - the only functions that can block execution in javascript are alert, confirm, and prompt. Of those, only alert can reasonably replaced with a non-blocking function.]

It's not impossible to achieve that effect, though. You can block off the entire page by positioning certain elements over the entire window, and then pop up your HTML based dialog box in the middle. And maybe your confirm dialog doesn't need to be modal? There are reasonable justifications for wanting a non-modal dialog box, and this provides a way to achieve those results.

Being modal (i.e. blocking further user interactions with the page) has nothing to do with it - only alert/confirm/prompt can block further execution of your script at the point where it is called, until it gets a yes/no answer - a useful substitute would require a much more complicated API involving a callback function, and could not be used as a drop-in replacement for the function we see here. (and, yes, a confirm dialog needs to block further execution, since the rest of the script will almost certainly depend on whether you clicked yes or no)

Not a WTF at all..

- Maybe it will contain something more in the future.
- Function could be called from other scripts the coder has no control over.

Only semi-wtf is the if( cond) return true else false thingy.

Something like this would make sense:

function confirmMessage()
{
if ( confirm('Here is a message we use 200 times but it changes sometimes') )
return true;
else
return false;
}

It's silly to throw a tantrum over the usage of an "if-then-else" instead of just "return confirm()" though.

Fuji:

If 'confirm' is in a third-party library, COM object, etc. for which source code is not available, then this wrapper function will give you a convenient place to set a breakpoint, or add debugging or logging code, especially when there can be hundreds or thousands of calls to it throughout a system.

....this is Javascript.. confirm is a built-in function. Had this been a call to some external object, then the confirm function would have had to be overridden to make this call, thus being possible to add a breakpoint/logging code there.

No, I agree with the theory that this is a refactor done by a twit. It does make sense to keep this function in place if it was called elsewhere in the app, trying to find every reference is a PITA and likely to break something. It's much easier to re-write like that, however, return confirm(msg); would have been the smarter thing to do....

I actually do something exactly like this in the code I'm currently maintaining. Not the pointless if/then part, but I do wrap confirm() in a function that does absolutely nothing but call it, then return the result.

Why? Because when you call confirm()/alert()/etc, it forces focus to the window that made the call. Since my app is a multi-window affair with all the logic in the main window and all the content in a popup window, this was bad. So instead whenever I need to pop up an alert or confirmation message, I call a little wrapper function that's embedded in every content page.

webhamster:

W:

Aye, this is just a way to abstract away message displaying. This way it's easy to change how a confirm dialog is displayed (be it the normal confirm() box, or some popup or similar (not that that wouldn't be a WTF)) without altering all the code that uses confirmation dialogs.

Exactly. If it's a method used in 200 places it's easier to make a change once than to make it 200 times through the whole codebase.

Because greap+sed or awk, and the two or three lines of bash to do all this (checkout the files, change, commit) is just so damn hard you know.

Oh, you say you do this development on Windows? Too bad.

Cheers!

Pingmaster:

No, I agree with the theory that this is a refactor done by a twit. It does make sense to keep this function in place if it was called elsewhere in the app, trying to find every reference is a PITA and likely to break something.

Why, oh why oh why... /me weeps.

What horrible experience you must have in order not to be able to grep the codebase and find all the references to a particular function. Yes, I agree that running the stack of your Fortran punched cards through the system in order to grep for things was expensive, but ya know we've made some progress since then...

Cheers!

Random832:

ydant:

It's not impossible to achieve that effect, though. You can block off the entire page by positioning certain elements over the entire window, and then pop up your HTML based dialog box in the middle. And maybe your confirm dialog doesn't need to be modal? There are reasonable justifications for wanting a non-modal dialog box, and this provides a way to achieve those results.

Being modal (i.e. blocking further user interactions with the page) has nothing to do with it - only alert/confirm/prompt can block further execution of your script at the point where it is called, until it gets a yes/no answer - a useful substitute would require a much more complicated API involving a callback function, and could not be used as a drop-in replacement for the function we see here. (and, yes, a confirm dialog needs to block further execution, since the rest of the script will almost certainly depend on whether you clicked yes or no)

Random832, you've got to the root of the issue. What a nice, non WTF-y comment. Thank you.

Cheers!

Kuba:

webhamster:

W:

Aye, this is just a way to abstract away message displaying. This way it's easy to change how a confirm dialog is displayed (be it the normal confirm() box, or some popup or similar (not that that wouldn't be a WTF)) without altering all the code that uses confirmation dialogs.

Exactly. If it's a method used in 200 places it's easier to make a change once than to make it 200 times through the whole codebase.

Because greap+sed or awk, and the two or three lines of bash to do all this (checkout the files, change, commit) is just so damn hard you know.

Oh, you say you do this development on Windows? Too bad.

Cheers!

You obviously work on a small team without a dedicated test group. On a large project, to make such a sweeping change requires one to:

1) Wait for all 200 files to become unlocked so you can check them out.

2) Make the change.

3) Unit test every single method that you just touched.

4) Check in each of 200 files, with appropriate release notes and defect references.

5) Listen to the test team gripe because they now have to regression test each feature that you just altered.

6) Endure the criticism of your manager because you just threw the project off schedule.

It's much easier to make the change in one spot, and the compiler will probably optimize it away anyway.

Ok, my WTF is with your steps...

1.) Wait for all 200 files to become unlocked... What kind of "Concurrent Versioning System" are you using? no one should be locking files!

3.) "confirmMessage" to "confirm" shouldn't need a unit test... unless you are real bad at typos.

4.) 1 check-in 200 files, 1 comment, 1 reference to the defect

5.) seriously, no. test once, but thats it.

6.) not likely

- that said, I'd like to see the change log on this function. I highly doubt it was ever more robust, or likely to be.

now to go drink some: cognac

Kuba:

What horrible experience you must have in order not to be able to grep the codebase and find all the references to a particular function. Yes, I agree that running the stack of your Fortran punched cards through the system in order to grep for things was expensive, but ya know we've made some progress since then...

Because perhaps you do not maintain all of the code that is calling this function, or because some of that code is in a code-freeze? Or maybe because you don't have time to rigorously test all 300 files that you just changed?

I look forward to the daily WTF complaining about the cowboy who wrote a script to change every file but made a typo in his regex and neglected to test the changes.

While I agree this could be a placeholder for future functionality or a hold over from functionality that was removed, you would still make if a one-liner in the damn method. return the result of the confirm() call, how hard can that be?

Now, the "greap[sp]+sed or awk" guy is either joking, or spends his days whipping up inconsequential perl scripts in his basement. But..

JTK:

You obviously work on a small team without a dedicated test group. On a large project, to make such a sweeping change requires one to:

1) Wait for all 200 files to become unlocked so you can check them out.

What large project is seriously using crappy file-locking source control like VSS? That sounds very, very painful, and would more or less make any sweeping refactoring impossible. IMO any non-trivial project needs an occasional housecleaning run like that, unless it is (A) immaculately designed, or (B) has the great luck of having requirements that aren't moving targets (ha!).

Agreed that this isn't a WTF, though. It's just a mildly lazy refactoring, probably trying to avoid the kind of pains you're talking about. And when you consider the pass-through get/set accessor methods or properties littering every Java / C# codebase in existence, it's positively mundane.

Kuba:

webhamster:

W:

Aye, this is just a way to abstract away message displaying. This way it's easy to change how a confirm dialog is displayed (be it the normal confirm() box, or some popup or similar (not that that wouldn't be a WTF)) without altering all the code that uses confirmation dialogs.

Exactly. If it's a method used in 200 places it's easier to make a change once than to make it 200 times through the whole codebase.

Because greap+sed or awk, and the two or three lines of bash to do all this (checkout the files, change, commit) is just so damn hard you know.

Oh, you say you do this development on Windows? Too bad.

Cheers!

Right, because we all know greap[sic]+sed, awk, and bash aren't available on Windows.

akatherder:

It's silly to throw a tantrum over the usage of an "if-then-else" instead of just "return confirm()" though.

It demonstrates a fundamental lack of understanding of how programming languages work. I really wouldn't want anyone who writes code like that on my project.

The WTF is not a function wrapped in another function, but rather
if (condition) return true;
else return false
instead of just
return condition;

http://www.google.com/codesearch?hl=en&q=+%22function+confirmMessage(%22+show:FJjXhKeHroI:DHGRpEzsbT8:sKIx5hAj7EU&sa=N&cd=1&ct=rc&cs_p=http://myonlineproject.googlecode.com/svn&cs_f=restaurant/quantri/javascript/validate.js#a0

jpd:

Now, the "greap[sp]+sed or awk" guy is either joking, or spends his days whipping up inconsequential perl scripts in his basement. But..

JTK:

You obviously work on a small team without a dedicated test group. On a large project, to make such a sweeping change requires one to:

1) Wait for all 200 files to become unlocked so you can check them out.

What large project is seriously using crappy file-locking source control like VSS? That sounds very, very painful, and would more or less make any sweeping refactoring impossible. IMO any non-trivial project needs an occasional housecleaning run like that, unless it is (A) immaculately designed, or (B) has the great luck of having requirements that aren't moving targets (ha!).

Agreed that this isn't a WTF, though. It's just a mildly lazy refactoring, probably trying to avoid the kind of pains you're talking about. And when you consider the pass-through get/set accessor methods or properties littering every Java / C# codebase in existence, it's positively mundane.

I think you would be shocked to discover how many large projects/companies use locking version control like VSS. In fact, I have yet to work at a company that did NOT use VSS.

Yeah, it sucks but I haven't had the issues with it that others seem to.

Seems like a great way to add in a breakpoint for a debugger.

Actually, this isn't as crazy as it looks.

One of the big problems (and features) in Javascript is that all of the variables can be any type. If you wanted a function that actually returned a boolean, independent of what you got in, you'd need to do something like this. Kinda silly, but sometimes what you have to go through to guarantee a particular type in JS.

--Ted

Why bother? The compiler does that for you.

Well, !! is still more concise than if... return true else return false. But, that's beside the point - confirm() itself _does_ always return a boolean.

And even if !! is ugly, I'd write a function like maybe function cbool(x){if(x)return true;else return false;} and call it everywhere else i need that. maybe also function cnum(x){return +x;} function cstr(x){""+x;} (more robust than toString because it works on integer primitives) - strings, numbers, and booleans are the only primitive types js has.

Joe H.:

Why bother? The compiler does that for you.

...what...compiler?

Since they were mentioned, something occured to me - this might be intended as something to hang a unit test on - you provide a different "confirmMessage" in the unit test to verify that it's called, and give it a specified return value, and you can use that to do noninteractive testing to make sure the code calls it and behaves as specified when you answer yes vs when you answer no. Of course, you could override confirm itself for this (I have overridden alert for my own debugging purposes in the past), but they might not know this.

freelancer:

Joe H.:

Why bother? The compiler does that for you.

...what...compiler?

Modern javascript implementations are more sophisticated than you think they are. There is a compiler. But, regardless, most javascript functions, and ESPECIALLY one that waits on user input, don't need to have every millisecond squeezed out of them.

brazzy:

akatherder:

It's silly to throw a tantrum over the usage of an "if-then-else" instead of just "return confirm()" though.

It demonstrates a fundamental lack of understanding of how programming languages work. I really wouldn't want anyone who writes code like that on my project.

Breakpoints.


You demonstrate a fundamental lack of understanding of how development processes work. I really wouldn't want you on my project, if you're going to use 4-lines of code and a lack of context determine the quality of a persons coding skills.

jpd:

What large project is seriously using crappy file-locking source control like VSS? That sounds very, very painful, and would more or less make any sweeping refactoring impossible. IMO any non-trivial project needs an occasional housecleaning run like that, unless it is (A) immaculately designed, or (B) has the great luck of having requirements that aren't moving targets (ha!).

Ever heard of clear case? It is painful...but seems to be popular with big companies.

For some reason, the bigger the company the less they can trust open source. I love subversion and use it for my personal projects, but I've never seen any big company use it. Maybe CVS but even that is rare.

Random832:

however, it is a little-known fact that it is IMPOSSIBLE to do a 'fancy confirm box' - the only functions that can block execution in javascript are alert, confirm, and prompt.

You can trivially just check the return value of a function and then proceed based on what it returned, use try/catch or - and this strikes me as the most sensible approach - just attach an event to the okay and cancel "buttons" on the DHTML confirm box - an event to a handler which is able to reference an object which it then performs an action when triggered (and so which executes code on confirmation, or doesn't, as the case may be).

It's perfectly straightforward to do this sort of thing in a cross browser compatible way. It relies on writing code in the right way to begin with though (i.e. not as large procedural scripts) .

I'm just surprised the coder was content with:

if ( confirm(msg) )
rather than
if ( confirm(msg) == true )

Though I suppose that might have given him a hint about the easier way to do it..

No joke -- less than 10 minutes after I read this today, I found this in our codebase:

private bool PageIsValid()
{
bool isValid = true;

if ( !Page.IsValid )
isValid = false;
return isValid;
}

I would truly love to believe that the "THEN" clause used to be much longer and complex and this was the result of a refactoring, so I searched through the source control -- nope. This gem was introducted in version #2 and has been there ever since.

Brillant!

Yeah, it ain't pretty. But wtf, if it ain't broke don't fix it.