11

For example, the following function loops through an array which contains the name and errors of an input field. It does this by checking the name of the validating field and then pushing the error info to the invalid fields array.

Is it better to be brief and write this:

addInvalidField (field, message) {
  const foundField = this.invalidFields.find(value => {
    return value.name === field.name
  })
  const errors = foundField.errors
  if (!errors.some(error => error.name === message)) {
    errors.push({ name: message, message })
  }
},

Or be more specific like this?

addInvalidField (validatingField, message) {
  const foundField = this.invalidFields.find(invalidField => {
    return validatingField.name === invalidField.name
  })
  if (!foundField.errors.some(foundFieldError => foundFieldError.name === message)) {
    fouldField.errors.push({ name: message, message })
  }
},
alex
  • 383

6 Answers6

24

If brevity can be sacrificed for clarity, it should. But if verbosity can be sacrificed for clarity, even better.

addInvalidField (field, message) {
  const foundInvalidField = this.invalidFields.find(x => x.name === field.name)
  if (!foundInvalidField.errors.some(x => x.name === message)) {
    foundInvalidField.errors.push({ name: message, message })
  }
},

When a variable only lives as long as one line it can be very short indeed. FoundInvalidField is used in three lines and is the focus of this work. It deserves an explanatory name.

As always, context is king.

candied_orange
  • 119,268
12

I actually favor your first code example.

It's clearly apparent what the code does just by reading it. By keeping the variable names as small as possible, you make the code even easier to read. More descriptive variable names would only be necessary if your functions were longer, your variables were more numerous and/or the variable(s) were used over a larger code scope.

It's because you've kept your functions brief that you can also keep your variable names brief. All other things being equal, less code is always better.

Robert Harvey
  • 200,592
4

I think I agree with Uncle Bob on preferring clarity without incurring in excessive verbosity. In the examples you show, I would say that the second one's intent is more clear without incurring in excessive verbosity. Also it would be easier to find that particular snippet when searching though the code base for invalidField than for value.


Well I'm quoting Clean Code here (skip it if you are fed up with Uncle Bob's preaching (which I'm not):

Use Intention-Revealing Names

It is easy to say that names should reveal intent. What we want to impress upon you is that we are serious about this. Choosing good names takes time but saves more than it takes. So take care with your names and change them when you find better ones. Everyone who reads your code (including you) will be happier if you do.


Avoid Disinformation

Programmers must avoid leaving false clues that obscure the meaning of code. We should avoid words whose entrenched meanings vary from our


Make Meaningful Distinctions

Programmers create problems for themselves when they write code solely to satisfy a compiler or interpreter.


Use Searchable Names

Use names that would help you do a grep -iIR whateveryouaresearching . (not a Clean Code, here CC only talked about single letter variables).


Avoid Mental Mapping

Readers shouldn’t have to mentally translate your names into other names they already know. This problem generally arises from a choice to use neither problem domain terms nor solution domain terms.


Use Problem Domain Names

When there is no “programmer-eese” for what you’re doing, use the name from the problem domain. At least the programmer who maintains your code can ask a domain expert what it means.


Tulains Córdova
  • 39,570
  • 13
  • 100
  • 156
1

I'd always opt to be more descriptive these days - IDE code completion means you won't have to write descriptive variable names so I can't see a downside.

Back in prehistory you had variable name restrictions and using meaningful variable names could actually incur a measurable cost (e.g. in BBC BASIC using the integer static variables A% etc. was way cheaper expensive than using a meaningful integer - and in a system with a 1MHz processor, saving a few clock cycles in a loop actually mattered)

mcottle
  • 6,152
  • 2
  • 26
  • 27
1

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.

Doc Brown
  • 218,378
0

There no correct answer in general in naming. Many people when given the exact same set of task will name the resulting functions and variables very differently. Of course you want others who read your code to understand, but longer doesn't always mean something is clearer. If your code is denser then it needs to be then it then it will take more time to understand even every line of your functions is as clear and descriptive as it can be.

Personally, I like the first example more. It's straight and to the point even with the variables not having as descriptive names as in the second example. Honestly, the variable names in the second example aren't much clearer then the first in my opinion and keeping the function brief makes it easier to understand the function itself.

At the end of the day, which is better will be up to you and whoever you're working with. After all, that's who will be reading it and maintaining it.

Dom
  • 169