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 })
  }
},
  • 1
    related (possibly a duplicate): Is there an excuse for short variable names? – gnat Nov 30 '16 at 7:33
  • Don't feel like writing an answer, but we always try to find a good compromise between name length and clarity. Short names may be obvious to the original programmer but not to everyone else. Long names can make the code a pain to read. There's a compromise in between. – MetalMikester Nov 30 '16 at 13:06
  • 1
    needs more comments – Ewan Nov 30 '16 at 16:52
  • Side note, and not sure if it's possible in your code, but if invalidFields etc. were stored in a map, not an array, this code would get much simpler. – user949300 Nov 30 '16 at 17:00
  • 1
    @alex In answer to your comment, If you can get or borrow a copy of Eloquent JavaScript by Marijn Haverbeke, (2014 version) go to pages 73-75 to see a great example of using a map instead of an array. Hope that helps. – user949300 Dec 3 '16 at 0:37
23

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.

  • 2
    +1 for "If brevity can be sacrificed for clarity it should. But if verbosity can be sacrificed for clarity, even better." Even when I strive for clarity almost in every circunstance. But that's definetively a quotable quote. – Tulains Córdova Nov 30 '16 at 12:37
13

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.

  • 2
    To piggyback on this, as a general rule, I prefer smaller variable names for my methods/functions. The only time that I would use a more verbose name is if there is a namespace conflict. If your function size is small though, this is also easier to accomplish. – unflores Nov 30 '16 at 10:27
  • 4
    I worked with someone who was really big into verbose names. variables and method names were basically full English sentences e.g. theResultOfMethodDoFooWithBar. The idea was that this was supposed to make things clear but it gave me a headache trying to parse (mentally) all of that fluff. For this example, we already know the method is about field validations. There are no other 'field' parameters. Using a name like 'validatingField' adds no clarity and obscures the important information when skimming. – JimmyJames Nov 30 '16 at 14:54
  • @unflores I also try to do that. validatingFields are form fields with validation. The original name was fieldWithValidation. It's really hard to find a short name for this one. I could call it just field but then it will have conflict with another field inside the method. – alex Dec 1 '16 at 1:30
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.


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)

  • 6
    The downside is not that you have to type a lot. Typing happens only once. The downside of names that are too long is that reading becomes harder. And that is insidious because reading happens many times over the life of the code base, and because people don't consciously notice the source of the problem because reading is so ingrained. – Kilian Foth Nov 30 '16 at 5:45
  • And if you have to spend time and context shifts trying to remember something about a variable because it wasn't descriptive enough will cost you more time :). – mcottle Nov 30 '16 at 5:48
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.

  • validatingFields are fields in a form that have validation. Initially, I named them fieldsWithValidation but it was a bit long. – alex Dec 1 '16 at 1:34
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.

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.