33
loading...
Harmless code and Obvious code - a Code Review Chronicles about Date validation
Code is read more often than it is written
This quote (google tells me it is from Guido van Rossum - the founder of Python) is a kind of variation of another quote from Clean Code: A Handbook of Agile Software Craftsmanship by Robert C. Martin:
“Indeed, the ratio of time spent reading versus writing is well over 10 to 1. We are constantly reading old code as part of the effort to write new code. ...[Therefore,] making it easy to read makes it easier to write.”
Both are very important for me and are kind of guidelines when I do code reviews.
Expecially this part "We are constantly reading old code as part of the effort to write new code. is crucial for me and it is why I often point out and ask questions about code that was not directly changed by the pull request.
Don't get me wrong, if I am reviewing one file during a Pull Request I am not asking the developer why other unrelated things were done in some way or asking to make changes to different lines of code that were not touched by that developer. That would be out of the scope of a Code Review.
But to understand the context of a change I need to read, and understand, well.. , the context of that change, that is the code around the modified lines.
To make better code reviews you have to question the change in the context of a larger system. And therefore you must read and understand the context.
Sometimes happens that I don't understand the context, or *the code that was changed or added, although perfectly written and formally correct, does not really makes sense, * because the context, in the form of surrounding code, old code, makes no sense. When I ask for clarification I sometimes get this answer:
Oh.. I don't know. I did not write that part. I was just asked to add a new functionality, or a new validation, or a new error handler, and that I did.
This is for me a very bad approach at coding. Which in the long run causes degradation in the quality of the code base.
"We are constantly reading old code as part of the effort to write new code.
How can you add some functionality, or modify it, if you didn't read the previous code, the surrounding code, if you did not understand the context?
Sorry, I might be a very mean and annoying reviewer, but I can't accept "I don't know, I didn't write that" as an answer ( and I am talking about 3 lines above the ones you added, not 3 classes or methods above).
I will give you a recent example, where one developer had to implement a specific rule/condition on a Date validation method, part of a legacy codebase.
The method was accepting a Date in a string format, and for various reason we were parsing the date string with a regex to make sure it is in a valid format ( we accept multiple Localized date strings like 28/11/2001 or 28.11.2001 ) and then we extract the date parts: day, month and year.
After creating the real date from the extracted values we already had some validation in place based on different specific requirements.
Just at the end of the method there was some code that looked like a final fall through validation. That left me puzzled.
After creating the real date from the extracted values we already had some validation in place based on different specific requirements.
Just at the end of the method there was some code that looked like a final fall through validation. That left me puzzled.
const validateDate = (dateString) => {
// some logic to parse and validate the string and extract the date parts.
const day = // value extracted from dateString
const month = // value extracted from dateString
const year = // value extracted from dateString
const date = new Date(year, month, day)
// lots of checks and conditions
// final fall through validation of the created date
return (
date.getFullYear() === year &&
date.getMonth() === month &&
date.getDate() === day
)I really could not understand why on earth we were creating a Date, and then checking the correctness of the date like that...
Was there something I wasn't grasping?
When I asked why we had that final validation, nobody was able to explain that. If not in translating the code in plain english.
const date = new Date(year, month, day)
return date.getFullYear() === year &&
date.getMonth() === month &&
date.getDate() === dayYes, we are making sure that the date we created with day, month and year has the correct day, month and year.
Yeah.. I get that. I can read code.
Did we maybe want to validate the constructor of the Date Class? What's the point in that?
Maybe, it was done because the values extracted by the regex could be "weird" or invalid?
I asked the developer if he considered that possibility and what would happen in such a case.
What happens if you do
new Date(2001, null, 5) // --> Jan 05 2001
new Date(undefined, 2, 12) // --> Invalid Date {}
new Date(2008, 1, false) // --> Jan 31 2008
new Date(2008, 1, "3") // --> Feb 03 2008
new Date(2008, 1, "nope") // --> Invalid Date {}In that specific case, if the point was making sure the Date was valid we could have simple check if the constructor was returning an Error or a DateInvalid message, why asserting the day, month and year?
The developer had no idea and no curiosity whatsoever to find it out, that's why he never questioned the code that he found in the method, and simply added _some more validation rules _ to it.
But when I find some code, I want to understand it, and if it doesn't make any sense I start to wonder if there might be some weird reason why that code must be there.
This is adding cognitive load, this is time consuming, this leads to defensive coding, this leads to messy, obscure codebases with code that nobody knows what it does and why it is there but nobody has the courage to remove...
This is adding cognitive load, this is time consuming, this leads to defensive coding, this leads to messy, obscure codebases with code that nobody knows what it does and why it is there but nobody has the courage to remove...
This is why I try to challenge developers during code reviews, I want them to develop analytical/critical thinking, and I want them to write simple, readable, obvious code: code that does not need any explanation, any comment. You read it, and you know what it does and why.
In that case, the reason for that seemingly stupid and unnecessary check was that the datestring parsed could have been for example
45.16.2009. The regex would have properly retrieved the 3 date parts and passed them to the Date constructor.But what happens if you do
new Date(2009,16,45) ?What happens if you pass as month a value higher than 11 (months are zero based) or a day that his bigger than 28 - if february - 30 or 31 for every other month? An error? a weird date?
No, the Date class will calculate automatically the right date, but counting the extra days and months. 16 will be May ( of the next year) 45 will be the 15 of the next month.
new Date(2009,16,45) // --> Mon Jun 14 2010So yes, to some extent, the validation we were doing, had a specific purpose, but it was very cumbersome and took quite some effort to understand why we have it.
It could have been done in a more straightforward ( and also more user - and developer - friendly) mode. Or maybe the presence of a simple unit test, would have made the code more understandable, and the purpose intelligible. And eventually caught immediately an error coming from a refactoring were that validation was removed.
Code must be simple and obvious, a dev should not spend minutes - or even seconds - trying to figure out what something is doing and mostly why the heck that code is doing that.
Nor should he simply shrug his shoulders because it does no harm.
Nor should he simply shrug his shoulders because it does no harm.
Not affecting performance, or not being executed at all, or not causing side effects or harm in any way is not a valid reason to keep some code into the code base.
33