The second variant looks makes me puzzled. When I only look at the signature, I wonder if the field is already known as beeing invalid? Or will it be validated first (as it is called validatingField
), to find out if it is really invalid?
So this not just redundant information here, the extra information seems to be somewhat misleading. This kind of "clarity" is not clearer, its the opposite.
Actually, when I saw your first function, it made me puzzled, too. I asked myself why the heck does your function just take a field, but then does not use it and searches for another one in invalidFields
? Looking for a field seems to make much more sense when there is just a fieldname is given, like this:
addInvalidField (fieldname, message) {
const foundField = this.invalidFields.find(value => {
return value.name === fieldname
})
const errors = foundField.errors
if (!errors.some(error => error.name === message)) {
errors.push({ name: message, message })
}
}
However, I guess Bob Martin would probably go a step further and make the code more verbose - for more clarity - in a different direction. A typical refactoring along the lines of the "Clean Code" book would probably look like this:
addInvalidField (fieldname, message) {
const foundField = findInvalidField(fieldName)
addMessageForInvalidField(foundField,message)
}
with three additional functions
findInvalidField(fieldname){
return this.invalidFields.find(value => { return value.name === fieldname })
}
addMessageForInvalidField(field,message){
const errors = field.errors
if (!doesErrorsContain(message)) {
errors.push({ name: message, message })
}
}
doesErrorsContain(message){
return errors.some(error => error.name === message)
}
It is debatable if it pays off to go that far with the single responsibility principle. It has actually some pros and cons. My personal point of view is that the original code is "clean enough" for most production code, but the refactored one is better.
When I knew I had to add something to the first variant so it would grow more and more, I would split it up to these smaller functions beforehand, so the code won't even start becoming a mess.