CODEWITHSUNDEEP
javascriptrefactoring
Gandalf StormCrow
Gandalf StormCrow

Reputation: 26192

writing more efficient code in javascript

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

Upvotes: 0

Views: 398

Answers (5)

Dave Ward
Dave Ward

Reputation: 60580

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

Upvotes: 3

Jason L
Jason L

Reputation: 2938

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

Upvotes: 10

blank
blank

Reputation: 18170

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!

Upvotes: 2

T.J. Crowder
T.J. Crowder

Reputation: 1074168

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.

Upvotes: 1

user159088
user159088

Reputation:

How about something like this:

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

Upvotes: 0

Related Questions