Readability and IF-block brackets: best practice

Question

I am preparing a short tutorial for level 1 uni students learning JavaScript basics. The task is to validate a phone number. The number must not contain non-digits and must be 14 digits long or less. The following code excerpt is what I came up with and I would like to make it as readable as possible.

if (
    //set of rules for invalid phone number
        phoneNumber.length == 0 //empty
    ||  phoneNumber.length > 14 //too long
    ||  /\D/.test(phoneNumber) //contains non-digits
) {
    setMessageText(invalid);
} else {
    setMessageText(valid);
}

A simple question I can not quite answer myself and would like to hear your opinions on: How to position the surrounding (outermost) brackets? It's hard to see the difference between a normal and a curly bracket. Do you usually put the last ) on the same line as the last condition? Do you keep the first opening ( on a line by itself? Do you wrap each individual sub-condition in brackets too? Do you align horizontally the first ( with the last ), or do you place the last ) in the same column as the if?

Do you keep ) { on a separate line or you place the last ) on the same line with the last sub-condition and then place the opening { on a new line? Or do you just put the ) { on the same line as the last sub-condition?

Community wiki.

EDIT Please only post opinions regarding the usage and placement of brackets. The code needs not be re-factored. This is for people who have only been introduced to JavaScript a couple of weeks ago. I am not asking for opinions how to write the code so it's shorter or performs better. I would only like to know how do you place brackets around IF-conditions.

Answer 1

message = invalid
if(phoneNumber.length > 0 && phoneNumber < 14 && /\D/.test(phonenumber)){
  message = valid
}
setMessageText(message)

essentially, invalid until proven valid is safer, but that is totally subjective.

also:

if(...){
}

is always better than:

if(...)
{

}

in javascript - this is because of semi-colon insertion, which might not be an issue right here, but could be in other cases, so it's better to maintain the same habits when working in the language.

lastly, I don't overload the code with more of this parentheses than I have to, especially when things are all "ANDed" - when there's mixed operator precedence, it's important to include the additional parentheses, because not everyone will want to think about that when reading the code.

This is all a bit subjective though. sorry.

Answer 2

I would refactor the logic for validating the phone number into a function:

function isValidPhoneNumber(phone) {
  if (phone.length == 0) return false;
  if (phone.length > 14) return false;
  return !/\D/.test(phone);
}

Or you can use the regular expression to check the length also:

function isValidPhoneNumber(phone) {
  return /^\d{1,14}$/.test(phone);
}

With a function for validating the phone number, the code gets simpler:

if (isValidPhoneNumber(phoneNumber)) {
  setMessageText(valid);
} else {
  setMessageText(invalid);
}

Or even:

setMessageText(isValidPhoneNumber(phoneNumber) ? valid : invalid);

Answer 3

tuh-MAY-toe

toe-MAH-tuh

barring bizarre habits, "readable" is what you're used to seeing

i would format your example like this:

//set of rules for invalid phone number:
//    - not empty
//    - not too long (14 characters)
//    - can contain only digits
if (phoneNumber.length == 0 ||
    phoneNumber.length > 14 ||
    /\D/.test(phoneNumber))
{ 
    setMessageText(invalid); 
}
else 
{ 
    setMessageText(valid); 
} 

because I prefer to see the logic explained all together instead of strewn about the code statements. And I like braces on a line by themselves to make the blocks stand out better.

But - as others have pointed out - for "ultimate" readability of this example, it should be refactored into at least a isValidPhoneNumber function