I haven’t written many blogs talking about code for a while, but my notes are stacking up; so time to post some blogs discussing code! I’ll try to explain them the best I can for the non-programmers, or those that find C# hard to understand.
There was one bug fix where the developer took several attempts to fix it. Taking several attempts to fix a bug illustrates that you either – don’t have a good understanding of:
- the requirements,
- or the codebase that you are changing.
If you make a change and it introduces a bug, then you make another change and it introduces another – instead of playing “whack-a-mole”, it’s worth considering if you are actually writing the correct fix. From my experience, you can often just take a step back, ask for help, and come up with a much simpler, and less risky fix.
In one of their changes, I saw evidence that it was possible that they didn’t understand what they were changing, or taking care in their work.
case ParentRecordRelationship.Grouped:
//when a record is grouped, the associated text is NOT altered - so this method should not be called
throw new ArgumentException(string.Concat("Invalid parent record relationship supplied: ", parentRecordRelationship));
So we have a comment, explaining that the existing requirement is that the “associated text” is not modified, so it is completely invalid to call this particular method which would change the associated text (other code not shown). Therefore, we throw an exception which would cause the system to show an error to the user.
The developer changed the code to this:
case ParentRecordRelationship.Grouped:
newAssociatedTextValue = CreateAssociatedTextValue(string.Format(_groupedWithFormatText, parentrecordOriginalTerm));
break;
//when a record is grouped, the associated text is NOT altered - so this method should not be called
//throw new ArgumentException(string.Concat("Invalid parent record relationship supplied: ", parentRecordRelationship));
So now he has allowed the text to be updated in this situation. Unless that really is a change in requirements, then this is the wrong thing to do. Also, instead of deleting the code that throws an exception, he has just “commented” it out (which is why it now shows in green), and also left in the actual comment saying that this scenario is invalid; which now adds to the confusion when the next developer reads it.
To me, that shows that the developer is uncertain it is the right thing to do, otherwise you would remove the comment and the code. Just “commenting” the old implementation out basically acts as a reminder that you might want to restore it. (You could easily just look at the history of the file to see the old code anyway, it isn’t like it is lost if you actually delete it in the first place.)
He also added to the confusion with this change
//take the 1st 12 characters of the format text
itemValueStartsWith = _combinedWithFormatText.Substring(0, 12);
it is now
//take the 1st 12 characters of the format text
itemValueStartsWith = _combinedWithFormatText.Substring(0, 9);
This is a good example of why writing comments for simple code is bad. The comment explains a requirement that it should take 12 characters, but you can see that it takes 9.
To me, this shows a lack of care, and lack of attention to detail – which are basic traits that I think are important for good software developers.
Stranger Coding Tings 