views:

193

answers:

5

Hi all, I'm trying to write more readable and more efficient code, I've always hated if if if chains .. is it possible to write the following statement in some other more "elegant" way like maybe using ternary ops :

if(text.length < 1){
 if(errtext.length > 1){
  errtext+="<br>";
 }
    errors = true;
    errtext += "Field cannot be empty";
}

Thank you

A: 

How about something like this:

errors = (text.length < 1);
if (errors) {
    errtext += (errtext.length > 1 ? "<br>" : "") + "Field cannot be empty";
}
dpb
It's pretty clear that this is just an excerpt from a larger set of error checks, any of which might set `errors`. So doing the unconditional assignment would wipe out the results of earlier checks (e.g., if `errors` was already true and this assignment was setting it false).
T.J. Crowder
I literally interpreted the question: 'write the following statement in some other more "elegant" way'. But yes, it is part of a larger set of error checks, so I agree with the down vote. But the question should be changed to "What is the best way to validate form fields in JavaScript?"... or something... so people don't take it literally... as I did :D
dpb
+3  A: 

One thing you can do is take advantage of the truthy nature of numbers greater than 0 and the falsey nature of numbers equal to 0:

if(!text.length) {
  if (errtext.length) {
    errtext += "<br />";
  }

  errors = true;

  errtext += "Field cannot be empty";
}
Dave Ward
Numbers less than zero still return true: >>> !!(-1) true >>> !!(0) false
Jason L
You're right. Updated.
Dave Ward
@Jason Presumably, `text` and `errtext` are strings, therefore, length will always be a non negative integer
Justin Johnson
His code wasn't wrong, just his statement that negative numbers return `false`. We wouldn't want to enlighten the OP with one hand and muddle him with the other. ;)
Jason L
+1  A: 

Your code as quoted seems quite clear. You could throw in a ternary (I'm assuming the check should be > 0, not > 1):

if(text.length < 1){
        errors = true;
        errtext += (errtext.length > 0 ? "<br>" : "") + "Field cannot be empty";
}

But that's not only less clear, but with a non-optimising compiler it's also less efficient. (Not that it matters in the context you're presenting here, which I'm guessing isn't a tight loop.)

Better options than a ternary (again, if there are a lot of these tests):

  • Always include the <br> in all of the error text cases, then remove the first one just prior to presentation. Much simpler if you have a lot of places you may be appending error text.
  • Build up the error messages in an array and then join them using <br> as your separator when presenting them.
T.J. Crowder
+11  A: 

Honestly, in most cases you want to use the if chains because they're more clear to a reader. There are a few exceptions, but not many.

Often if your code is awkward, your fundamental approach is wrong. Try this:

function validate()
{
  errs = [];
  // ...
  if (!txt.length) {
    errs.push("Must have text, man!");
  }
  // ...
  return errs.join("<BR>");
}

It bears mentioning that strings are immutable in Javascript. If you += on strings, you're creating a lot of intermediary strings, which can slow down performance.

E.G.

var msg = "Tom";
msg += ", Brad";
msg += ", Sam";
msg += ", Rick";

This ends up creating the following strings in memory:

"Tom"
"Brad"
"Sam"
"Rick"
"Tom, Brad"
"Tom, Brad, Sam"
"Tom, Brad, Sam, Rick"

Where as the following:

var msg = ["Tom", "Brad", "Sam", "Rick"];
var msg = msg.join(", ");

creates the following in memory:

"Tom"
"Brad"
"Sam"
"Rick"
"Tom, Brad, Sam, Rick"

Wouldn't make a big difference in this case, most likely, but it's good to get into the habit of creating arrays of strings and using the join method. In larger text manipulation/generation problems, it can cause a huge slowdown.

EDIT:

If the calling function needs to check for the presence or absence of errors, you can always do an if on the .length of the returned string. If you want to be more explicit, you can always do this:

if (!errs.length) {
  return null; // or false, or a constant...
}
return errs.join("<BR>");

and then add a check in your calling code (=== null, etc.)

Jason L
yes: array.join is so much better that an if statement in a loop!
Rob Fonseca-Ensor
I can't tell if Rob Fonseca-Ensor is being sarcastic or not. Nonetheless, pushing to an array is more efficient than repeatedly concatenating strings most of the time.
McPherrinM
If you need an extra "<BR>" at the end, I would usually just add an `errs.push("");` line before the return statement.
Jason L
+2  A: 

How about making a class of Error and some kind of Errors collection with an add method?

Your error collection would know how to add br tags and your calling code would be all validation logic instead of validation, message text and markup all mixed into one.

You could check to see if you errors collection has any entries, insted of having a flag.

You could even reuse the error in other places in your application!

Bedwyr Humphreys
Don't use the name `Error` though, because that's the built-in "class" for exceptions. You could just use that class of course instead of writing your own.
Matthew Crumley
@Matthew - Not being that familiar with javascript I wasn't aware of that. Fancy writing an answer incorporating that info? I'll delete this and you can have the vote.
Bedwyr Humphreys