Tinselcity

Sometimes you find something that doesn't quite make that much sense. It may be some condition that is literally impossible to reach, or some function that doesn't apparently do what you expect or that you just can't figure out why someone would want to do what the function does.

The other day I ran across the blockForm function.

blockForm - a Rather Boring Story of Code Archaeology

I found blockForm being called before submitting a form. This did make some sense. It looked like the typical situation where, to avoid double submissions someone would disable the submission buttons or some similar thing. Maybe not the best solution, but at least reasonably frequent so as not to be weird.

The code speaks

As I've landed on this code recently, I spend a fair amount of time reading that sort of code, just to better understand it. So I searched for this blockForm function and found two things. This is the code for the function:

function blockForm(oForm)
{

    return true;
}

Oh, it does nothing.

Something doesn't seem quite right, but fortunately there's a comment just above it…

/**
    This function disables all fields in a form. It's used when the user
    submits a form and we don't want them to touch anything until we're done
    with the request.
    <html:form ...
     onsubmit="blockForm(this);confirmarCambioPagina = false;">
     or, if the submission is done from JavaScript, calling blockForm(form)
     before doing form.submit();
*/

So, at some point in time, this function did something. It's not exactly what I guessed it would do, but close enough: Apparently it blocks the inputs of the form while waiting for the submission to go through.

My curiosity now piqued, I go through the Subversion history log in search for the original code. And sure, five years ago, the blockForm function was emptied of all its content except for that lone return true;. The original code is fairly standard: It goes through all the form elements and sets inputs to readonly and buttons to disabled. It does have some peculiarity as it includes a final…

    //DisableEnableLinks(true);
    //settimeout hace que se ejecute en otro hilo y no da conflicto
    setTimeout('DisableEnableLinks(true)',0);

But this is merely anecdotical. The thing is that for seven years this function here did its thing. And then since five years ago it does nothing. Which raises two main questions. On the one hand, why was this needed in the first place? And on the other, why did it later become unnecessary? But beyond these questions, the search I did to find blockForm brings up a more immediate concern.

We don't talk enough

Apparently the blockForm function is extremely widely used. Not only so, it seems that it was widely used when it worked, but it has also been widely used in the five years since it was lobotomized. Some of it, probably, has happened through careless copy-pasting, a common practice in the project. But to some extent the blockForm call has been intentionally introduced in many places.

What this means is that, when the function was rendered useless, nobody told the rest of the team that they no longer needed to call it. And so, they kept superstitiously calling it every time. The call to blockForm appears about 4500 times throughout the application.

Back to our main program

blockForm was created fairly early in the project's life. In fact, it begins its journey at revision 1466. This is about six months into the project. The commit message simply mentions avoiding double submissions.

As I explained before, this is a fairly common solution to a fairly common problem. The solution is neither particularly good nor particularly bad. It can work out though it may sometimes cause other worries like blocking a form, the submission not going through and leaving the user in a state where they just can't try again. But all in all, I insist, the approach is common enough.

But then, seven years later, when the function “dies”, the commit message isn't as clear as it could be. It explains that the function could indeed leave a page in a blocked state but it also mentions that it is “no longer necessary with the new beforeunload events”. What does this mean?

To be honest, I haven't been able to obtain a definitive answer to that, but everything hints that it doesn't actually mean anything. All other surrounding elements seem to indicate that, simply put, the original problem was never that important in the first place. And on the other hand, the potential to block a page and leave the user confused and frustrated was realized a number of times.

Symptomatic

The problem with blockForm may seem, in itself, not that big. After all, it's just one function call. It's useless, sure, but it's also apparently harmless, isn't it?

Well, to some extent that may be true, but the real problem is that blockForm is representative. It is not just one useless, harmless function call. What it is is code littering.

The code is generally littered with commented-out code and, worse, unused code. This last part is especially prevalent in client code (HTML, JS, CSS). Not only nobody bothers explaining that certain functions are not needed anymore, also, nobody ever bothers cleaning up after themselves.

Copy-pasting runs wild through the whole thing, and there's an evident feeling of superstition. This was evident whenever I asked for more detailed explanations about why some things were the way they were. The typical answer to that is “I don't really know; that's how it's done.”. A lot of things are left there just in case they are needed again some time… and due to a clear mistrust in the SCM tool nobody even imagines they could -if needed- recover it from an old version stored in Subversion. This mistrust is patently clear in their usage of SVN. There's one person in charge of branching, merging, and… setting up people to work on a certain branch. Which never happens anyway, but it clearly indicates that developers simply don't actually use subversion other than to commit to the same shared development branch.

There's yet another problem that this communication problem evidenciates. The bus factor for the project is one1). The project is in fact a One Man Band disaster. There's this one person that the project simply couldn't work without at all. This person accumulates -and generally only shares restrictedly- most of the technical information about the project. He, in fact, speaks of the code as he would speak of himself. “Here I do this…” to explain what the code does. And in an effort to, ahem, be more Agile (*sigh*), they avoid most sorts of documentation. There are some general Word documents, most of them obsolete and untouched since 2009. And more recently, they are moving to abbandon Enterprise Architect, which the analysts use to (mis)communicate with the developers. In it's place they want to take an approach were requirements and functional details are transmitted orally as was done in the Prehistory. Not that EA is a good tool, of course, but it is, at least, a tool.

Technical illness

But the problem is not only one of ill communication, a “people problem”. It's also a technical one. blockForm, particularly that block verb in its name, is also a good exponent of a general technical problem that plagues the application.

Most of the interface is built in a synchronous, blocking way. In particular, the majority of data requests (done through jQuery) have the async: false flag set2). In fact, a large number of these seem to have acquired -through CPI3)- a most charming comment:

    async: false, // We need to make this synchronous because we have to wait for the response

We need to, you see, because if we don't, then how do we get our data? Somewhere else in the code, someone set up some semaphores to block the interface for one second after a certain action runs (saving or deleting an event). Notice the detail: it doesn't block while the action is running; that's already taken care of using, of course, async: false. But then after the response is received and processed, they block the interface for 1 additional second just because.

Clearly, nobody ever bothered to understand how to do things asynchronously. They just realized that you could just block and everything would be “fine”. Fine indeed. A fine mess of tightly woven synchronous processes that is extremely expensive to disentangle because everything is assumed to be carefully dependent on order. Sure, the user suffers these small, unresponsive freezes, but hey, such is life.

99 problems but blocking ain't one

This is not, unfortunately, the only big problem in the application. The code strains under a handful other problems on a heavily deformed layer architecture where some layers are spread thin like plastic film while others are commonly pierced through. But, that's a different matter, unrelated to our subject today, blockForm, the function that returns true.

As a conclusion to all this, I've just added blockForm to a list of things to just delete whenever I see them. You know, cleaning up instead of just throwing more garbage on it. Most probably nobody will care, or even notice, but I will.

confirmPageChange - the comeback

Oh! I thought this would be all about blockForm, but then, today, I found about the confirmPageChange flag variable. It is, of course, a global variable defined in the same generic forms.js file where blockForm lived.

There are, according to the search tool, about 3014 occurrences of confirmPageChange. Out of these, 3010 set the flag to false and, naturally, the initial declaration sets it to true The other 3 occurrences are in a comment which optimistically explains how the flag must be set to false… “to avoid verifying changes when submitting”, whatever that means.

Turns out this was used in that onbeforeunload function mentioned somewhere in relation to blockForm 's demise, but which have since become obsolete themselves too. It's not used anymore since about 5 years ago. More precisely, it was never used. Right after being introduced, the onbeforeunload control is removed and never used again.

(Un)surprisingly, the 3000 confirmPageChange = false; that serve no purpose, that never served any purpose, still superstitiously remain til today.

Update

I removed the whole thing. Most people were slightly surprised when I told them I had removed blockForm, but assumed that blockForm was actually working and that I removed it without any particular reason. A couple of them didn't actually believe me when I told them the function wasn't actually doing anything. Whatever.

1)
Maybe even less than one; meaning that large pieces of knowledge are already lost without needing to kill anyone
2)
And of course, the rest explicitly set async: true just in case because you can never be too sure
3)
Copy Paste Inheritance