Code review: before & after; which do you prefer?

1

I’m working on form validation using VueJS for the 1st time. There’s a ton of fields I need to check.

Both format works… which is more readable? or easier to understand for you?

typical if…then formatting, 6 lines of code

if (!this.email) {
  this.errors.push("Email address is required.");
  this.emailError = true;
} else {
  this.emailError = false;
};

which I guess can also be shortened to 4 lines of code

if (!this.email) {
  this.errors.push("Email address is required.");
  this.emailError = true;
} else this.emailError = false;

or this alternative?

this.emailError = (!this.email) ? true : false; 
if (this.emailError) { this.errors.push("Email address is required.") }

// which can be further simplified to
this.emailError = (!this.email) ? true : false; 
if (this.emailError) this.errors.push("Email address is required.");

or do you have other suggestions?

2

I kind of like the last one. Couldn’t you use just this.emailError = !this.email;?

1 Like
3

Of course, thank you for that tip! Implemented it and still works, the classes are updated correctly.

<td v-bind:class="{error: zipError}">Zip</td>
4

Go number 1 if you want to keep it simple. Yea its more typing, but everyone knows whats up at a quick glance how the code will break down. (If this, or that)

Throw out the else on number 2, and this is more or less the same as number 1. I hate the idea of having an else for a single line statement, add the brackets with else or don’t use the else.

I personally dislike the single line if statement, it can screw you up when your not paying attention. Plus were talking about validation right? Validation is already touchy enough, no need to make things more complicated. You might want to add another line of logic under the if and might forget you don’t have brackets, and funky stuff start happening. If your going to use the brackets, put your statements on their own line, don’t wrap the single statement on the same line.

Finally this:
this.emailError = (!this.email) ? true : false; could technically be written as just:
this.emailError = !!this.email; I hear this syntax trips people up, so if you find it to odd then don’t use it. (I use this a lot to please typescript, and makes perfect sense once you think about it)

5

Thanks brad for the feedback.

#1 Yeah, this is the classic form and very easy to understand. This was my the first solution, but I end up with a wall of code. And I have 3 forms on this page (on different tabs), and each form has lots of fields so I can see the JS portion just growing.

Well, I manage to reduce it even further down to 1 line of code per field of validation.

Tested it, and it works just the same as the long format #1. I added a comment just in case I need to revisit this page several months down the road.

// Note: Variable assignment inside the parenthesis! Not equality operator. 
if (this.nameError = !this.name) this.errors.push("Your name is required.");          

if (this.churchError = !this.church) this.errors.push("Church name is required.");

if (this.positionError = !this.position) this.errors.push("Your church position is required.");

if (this.addresstypeError = !this.addresstype) this.errors.push("Address type is required.");

if (this.addressError = !this.address) this.errors.push("Street address is required.");

if (this.cityError = !this.city) this.errors.push("City is required.");

if (this.stateError = !this.state) this.errors.push("State is required.");

if (this.zipError = !this.zip) this.errors.push("Zip Code is required.");

if (this.phoneError = !this.phone) this.errors.push("Phone is required.");

if (this.workphoneError = !this.workphone) this.errors.push("Work phone is required, or use home phone number.");

if (this.emailError = !this.email) this.errors.push("Email address is required.");