3 ms·

This reminds of a piece of code I saw during my training, a web application written in Perl, by a Java guy, that I had to knead into shape. When I was almost f

by krylon 11y ago

This reminds of a piece of code I saw during my training, a web application written in Perl, by a Java guy, that I had to knead into shape. When I was almost finished with it, the one ugly element that remained was a C-style for loop with a head that filled one and a half lines (about 150-160 characters) of seriously cryptic code, and its body was not much better. I had been explicitly ignoring this loop while I was working on the application, because I had no clue what it did, and the app was working more or less they way it should. But when I was finished slightly ahead of schedule, I could not help but use the free time I had left trying to figure out what the for-loop did. Otherwise, the code I had been massaging for a couple of months now was pretty pretty, but the prettier it had become, the more this ugly for-loop stuck out. I started by adding a simple logging statement to its body that simply announced the fact that the loop was executing an iteration. Then I played around with the application for a while. Eventually, I took a look at the log file to see how often the loop had executed, and was rather baffled - the loop had executed a total of zero times. I played around with the app some more, trying my worst to make that loop run at least once (which was, admittedly, hard, because I had no clue what it was for). Two hours later, I gave up and removed the loop. I finished my training about eight months later, and at least during that time, I did not hear of it again. (I really would have liked to know where that loop came from, though.)

greenyoda 11y ago
"I played around with the app some more, trying my worst to make that loop run at least once (which was, admittedly, hard, because I had no clue what it was for). Two hours later, I gave up and removed the loop." That seems very risky, and I'd never have allowed that change to pass a code review. Just because you couldn't figure out how to make the loop execute doesn't mean that there aren't any paths through the code that would cause it to execute. The only safe way to remove it would be to prove that it can't ever execute -- e.g., the loop only executes if the variable foo is true (the condition is "foo && ..."), but foo is initialized to false and isn't modified anywhere else in the code.
- Nadya 11y ago
  >The more modern type of reformer goes gaily up to it and says, “I don’t see the use of this; let us clear it away.” To which the more intelligent type of reformer will do well to answer: “If you don’t see the use of it, I certainly won’t let you clear it away. Go away and think. Then, when you can come back and tell me that you do see the use of it, I may allow you to destroy it. Source: http://www.chesterton.org/taking-a-fence-down/ http://www.chesterton.org/taking-a-fence-down/
  - MaulingMonkey 11y ago
    A good general guideline, but on occasion attempting to trigger the trap with a stick is a useful exercise. Just make sure you use version control ;)
  - tmd83 11y ago
    In general true but sometimes it's almost impossible to prove that it is actually unwanted or plain wrong and you need to take some risk for the bigger benefit. It's not something to do lightly but it is actually needed sometimes. Our application has bits that hasn't been touched in years and no devel exists with experience in the said (problematic code) and if new development is needed and if that bit puts the new work at risk sometimes you have to make some judgement call and take the calculated risk.
- krylon 11y ago
  First of all, I was a trainee. I do not know if that ever even entered production use. Secondly, the problem the web application was intended to solve was really trivial: Service managers used it to register customer machines that were intended to send email; they would enter the IP address of the server, the DNS domain of the sender and the account number of the customer to bill. The web application validated the data (fairly crudely - the spec I was given defined an account number as "consisting of decimal digits", and an email address as "containing an @-sign"), entered it into a database and sent an email to the firewall team so they would adjust their rule sets. Me and my supervisor looked at that loop intensely, and there was no explanation we could come up with why it would even be there in the first place. (The whole thing was less than a thousand lines of not-very-dense Perl code, plus a few hundred lines of HTML templates. It's not like this was some highly complex piece of code.)

by krylon 11y ago

greenyoda 11y ago

"I played around with the app some more, trying my worst to make that loop run at least once (which was, admittedly, hard, because I had no clue what it was for). Two hours later, I gave up and removed the loop." That seems very risky, and I'd never have allowed that change to pass a code review. Just because you couldn't figure out how to make the loop execute doesn't mean that there aren't any paths through the code that would cause it to execute. The only safe way to remove it would be to prove that it can't ever execute -- e.g., the loop only executes if the variable foo is true (the condition is "foo && ..."), but foo is initialized to false and isn't modified anywhere else in the code.

Nadya 11y ago
>The more modern type of reformer goes gaily up to it and says, “I don’t see the use of this; let us clear it away.” To which the more intelligent type of reformer will do well to answer: “If you don’t see the use of it, I certainly won’t let you clear it away. Go away and think. Then, when you can come back and tell me that you do see the use of it, I may allow you to destroy it. Source: http://www.chesterton.org/taking-a-fence-down/ http://www.chesterton.org/taking-a-fence-down/
- MaulingMonkey 11y ago
  A good general guideline, but on occasion attempting to trigger the trap with a stick is a useful exercise. Just make sure you use version control ;)
- tmd83 11y ago
  In general true but sometimes it's almost impossible to prove that it is actually unwanted or plain wrong and you need to take some risk for the bigger benefit. It's not something to do lightly but it is actually needed sometimes. Our application has bits that hasn't been touched in years and no devel exists with experience in the said (problematic code) and if new development is needed and if that bit puts the new work at risk sometimes you have to make some judgement call and take the calculated risk.
krylon 11y ago
First of all, I was a trainee. I do not know if that ever even entered production use. Secondly, the problem the web application was intended to solve was really trivial: Service managers used it to register customer machines that were intended to send email; they would enter the IP address of the server, the DNS domain of the sender and the account number of the customer to bill. The web application validated the data (fairly crudely - the spec I was given defined an account number as "consisting of decimal digits", and an email address as "containing an @-sign"), entered it into a database and sent an email to the firewall team so they would adjust their rule sets. Me and my supervisor looked at that loop intensely, and there was no explanation we could come up with why it would even be there in the first place. (The whole thing was less than a thousand lines of not-very-dense Perl code, plus a few hundred lines of HTML templates. It's not like this was some highly complex piece of code.)