0

I have a function that validates few different conditions. Here is the example of my function:

function checkData() {
  var errorMsg = "",
    fld1 = 0, //Number(document.getElementById('fld1').value),
    fld2 = 5, //Number(document.getElementById('fld2').value),
    fld3 = 1, //Number(document.getElementById('fld3').value),
    fld4 = 0; //Number(document.getElementById('fld4').value);

  if (!fld1) {
    errorMsg += "Error 1\n\n";
  }

  if (fld1 === fld4) {
    errorMsg += "Error 2\n\n";
  }

  if (fld2 > fld4) {
    errorMsg += "Error 3\n\n";
  }

  if (fld3 > 3) {
    errorMsg += "Error 4\n\n";
  }

  if (errorMsg !== "") {
    var check = confirm(errorMsg + "\n Do you want to submit the form?");

    if (check) {
      return true;
    } else {
      return false;
    }
  }

  return true;
}
<button onclick="checkData();">Click Here</button>

In the example above I hard coded some values for testing purpose. However, I'm wondering if I can refactor this code and find the better way of achieving the same result? Would ternary operators fit better? Or there is another way to get this to work? Thank you.

adiga
  • 34,372
  • 9
  • 61
  • 83
espresso_coffee
  • 5,980
  • 11
  • 83
  • 193
  • 1
    In this use-case I think the 'multiple-ifs' solution is quite clear so it is the one to use. – MarcoS Feb 01 '19 at 15:38
  • 2
    @MarcoS is correct. Doing anything fancy, here, would only confuse anyone else trying to figure out what the code is doing – Leroy Stav Feb 01 '19 at 15:40
  • Agree with @MarcoS. I'm voting to close this because the code looks fine as it is. Even if any changes are suggested they'll be trivial which won't add any value to the site or future visitors. – adiga Feb 01 '19 at 16:07

3 Answers3

3

In this use-case I think the 'multiple-ifs' solution is quite clear so it is the one to use.

If you want to optimize a bit, I can only suggest

        if(check){
            return true;
        }else{
            return false;
        }

to become

return !!check;

(the two exclamatives simply cast any object to a boolean vale :-))

MarcoS
  • 17,323
  • 24
  • 96
  • 174
  • Why double `!` ? – Jose Rojas Feb 01 '19 at 15:41
  • You convert any argument (`null`, `undefined`, `0`, etc.) into a boolean with `!check`. The problem now is that you inverted the boolean, so if it evaluated to `true` before, you have `false` now. Using two exclamation marks, `!!check` will get you the initial condition of your boolean. See this answer for more details: https://stackoverflow.com/a/784946/4791226 – Michael Gecht Feb 01 '19 at 15:44
1

The whole check variable is pointless. So return confirm is all that you need

function checkData() {
 var errorMsg = "",
  fld1 = 0, //Number(document.getElementById('fld1').value),
  fld2 = 5,//Number(document.getElementById('fld2').value),
  fld3 = 1,//Number(document.getElementById('fld3').value),
  fld4 = 0;//Number(document.getElementById('fld4').value);

 if(!fld1){
  errorMsg += "Error 1\n\n";
 }

 if(fld1 === fld4){
  errorMsg += "Error 2\n\n";
 }

 if(fld2 > fld4){
  errorMsg += "Error 3\n\n";
 }

 if(fld3 > 3){
  errorMsg += "Error 4\n\n";
 }

    return errorMsg !== "" ? confirm(errorMsg + "\n Do you want to submit the form?") : true

}
<button onclick="checkData();">Click Here</button>
Alen.Toma
  • 4,684
  • 2
  • 14
  • 31
0

You could refactor your if statements using the ternary operator. But chances are that it would make your code far harder to read. You could replace

if(check){
    return true;
}else{
    return false;
}

With just return check; as this is a boolean statement anyway.

Also, as far as readability goes it would be nice to label your field variables something more meaningful, as knowing that fld2 should always be greater than fld4 isn't immediately obvious from the name.

And if you don't care about highlighting the specific error codes then you could of course merge some of your checks together and just return false without the error codes specified, but I suspect you will want to keep that functionality.

Adam
  • 1,724
  • 13
  • 16