Over in Moodle bug MDL-42974, Derek Chirnside asked "What is it about a hack that makes it 'horrible'??". I had described the code sam wrote to fix that issue in those terms, while at the same time submitting it for integration. It was a fair enough comment. I had helped sam create the code, and it was the kind of code you only write to make things work in Internet Explorer 8.
Although "Horrible hack" is clearly an aesthetic judgement, and therefore rather subjective, I think I can give a definition. However, it is easier to start by defining the opposite term. What is "Good code"? Good code should have properties like this:
- It works: It does what it is supposed to.
- It is readable: Another developer can read it and see what it is supposed to do.
- It is logical: It does what it is supposed to do in a way that makes sense. It is not just that a developer can puzzle out what it does, but it is clear that it does just that and nothing else.
- It is succinct: This is a companion to point 3).
- It is maintainable: It is clear that the code will go in working in the future, or if circumstances do change, it is clear how the code could be modified to adapt to it.
Note that property 1) is really just a starting point. It is not enough on its own.
A horrible hack is code that manages little more than property 1. I think sam's patch on MDL-42974 scores a full et.
- It is not at all obvious what the added code it for. Sam tried to mitigate that by adding a long comment to explain, but that is just a workaround to the hackiness of the code.
- There is no logical reason why the given change makes things work in IE <= 8. We were just fiddling around in Firebug to try to find out how IE was going wrong. Changing the display property on one div appeared to solve the display problem, so we turned that into code. We still don't really understand why. Another sign of the illogicality is the two setTimeout calls. Why do we need those two delays to make it work? No idea, but they are necessary.
- The whole chunk of added code should be unnecessary. Without the addition, it works in any other browser. We are adding some code that should be redundant just to make things work in IE.
- We don't understand why this code works, so we cannot understand if it will go on working. In this case, lack of maintainability is not too serious. The code only executes on IE8 or below. In due course we know we can just delete it.
Normally, you would wish to avoid code like this, but in this case it is OK because:
- The hack is confined in one small area of the code.
- There is a comment to explain what is going on.
- It is clear that we will be able to remove this code in future, once usage of IE8 has dropped to a low enough level.
At least, we hope that the Moodle integration team agree that this code is acceptable for now. Otherwise, we wasted our time writing it.
