Just 1 line of code.
I was fixing/debugging/enhancing some code and came across this 1 line of code.
PrintSendStatement(1,sessionStatementGrid);
The line was so good yet so bad all at once. This is what I was able to work out just by looking at this line of code.
Firstly it was calling a procedure named PrintSendStatement. Which seems like a well named procedure I can guess exactly what it does. The code that prints statements and the code that sends statements must be very similar so one procedure that does both seems reasonable. I have no idea the scope of the called procedure but from it's name it does not seem relevant. Score: +1
The second parameter is the number 1. This just kills me. I can see another line that calls the same procedure with a zero as the first parameter. Without further digging I have no idea what zero and one mean other than guessing one prints and the other one does not. A constant would have been super cool here. It would have taken maybe 30 seconds for the original developer to do. Now it will take me and every other developer for the rest of time 30 seconds to look it up. Score -2
The third parameter is a grid. Which surprised me as I would have expected a different colection or a single variable to be passed. A grid is a weird collection to use so it told me something. It told me that PrintSendStatement probabley had a very local scope and it needed a grid as a parameter. No-one would choose a grid as a parameter so it must be required. Why? Grids allow the user to select or multiselect records. I can infer that PrintSendInvoice will print or send the select records in the grid. Score +1. But I'm taking off another point because if PrintSendInvoice is local then constants really should have been used, the developer can't fall back on the excuse 'I copied the code and that is the way it was'.
Total Score : -1.
So now I have to dig into PrintSendStatement and see what it does. I didn't want to do this. I was fixing something else. I really need a timesheet code to charge back the original developer and bill him/her.
Should I put constants in now?
-
Yes: That will save time for all developers going forward.
-
No: I'm not here to touch that code. Plus I'd have to search for all the ones and zeros to keep things consistent. This could take a while.
-
Compromise: Just add comments at this location to help the next developer without creating any new bugs.
What would you do?