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

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

• javascript
• refactoring

• Shouldn't that be if(errtext.length > 0)? (0, not 1)? – T.J. Crowder Commented Dec 17, 2009 at 15:43
• IMO you should also tag your question with "refactoring" – user159088 Commented Dec 17, 2009 at 15:50
• if you really meant efficiency you might want to take a look at how to profile your javascript using Firebug – Perpetualcoder Commented Dec 17, 2009 at 17:18
• 1 really you should separate your validation logic from the error message rendering – Andrew Bullock Commented Dec 17, 2009 at 17:32

5 Answers 5

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.)

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";
}

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!

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 piler 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.

How about something like this:

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