41

If I have an if statement that needs to meet these requirements:

if(cave > 0 && training > 0 && mobility > 0 && sleep > 0)

Is there any way to say that all of them are bigger than zero? Just for more efficient DRY code?

Something like:

if(cave, training, mobility, sleep > 0)
CodingIntrigue
  • 75,930
  • 30
  • 170
  • 176
Oliver Klein
  • 589
  • 5
  • 12
  • 1
    there WAS a good answer if those values can only be 0 or positive (never negative, or never VERY VERY large) – Jaromanda X Aug 06 '15 at 12:23
  • @JaromandaX i thought of that too! – Vibhesh Kaul Aug 06 '15 at 12:24
  • 3
    whats wrong with `if (cave && training && mobility && sleep)`? – Alex Aug 06 '15 at 12:27
  • 3
    @Alex works if values can never be negative – Jaromanda X Aug 06 '15 at 12:28
  • Yeah the negative thing is an issue! Thanks though – Oliver Klein Aug 06 '15 at 12:29
  • what if there were more variables involved then... – EugenSunic Aug 06 '15 at 12:31
  • 5
    @Alex if one of the values was 0 and another was 4 it'd pass your test (and fail OP's requirements). – James Donnelly Aug 06 '15 at 12:31
  • 1
    @JamesDonnelly true :P – Alex Aug 06 '15 at 12:32
  • 13
    This thread is replete with over-engineered and cute code samples. There is _nothing_ wrong with your if statement. If you have more than four conditions, I'd suggest using intermediary flags (`valid_location && valid_status`). – isanae Aug 06 '15 at 17:46
  • 36
    What's wrong with the original statement? 1. It's simple. 2. It's concise. 3. It's clear. 4. It does exactly what you want it to do. No other suggestion on this page can satisfy all those requirements. – Mohair Aug 06 '15 at 17:15
  • 7
    I don't see using `&& > 0` several times in an if statement once to be a violation of DRY. If you have to use that same set of conditions in multiple place, then place it in a function or in the prototype for that "class". Being clever will only make things more annoying to debug for others. – Travis J Aug 06 '15 at 19:31
  • 2
    The obvious solution is the best one: `if(cave > 0 && training > 0 && mobility > 0 && sleep > 0)`. If it makes the code easier to read, you should pull the conditional into a separate function. – BlueRaja - Danny Pflughoeft Aug 06 '15 at 21:22
  • 1
    For whatever reason, this is the [kind](http://stackoverflow.com/q/236129) [of](http://stackoverflow.com/q/6116474) [question](http://stackoverflow.com/q/18347033) that brings out the worst answers out of programmers. It's as if we were compelled to find unusual and clever answers. Your job is to write code that is easy to understand. One-liners won't give you more points (outside of stackoverflow, at least). – isanae Aug 06 '15 at 22:38
  • 2
    @isanae , thats the thing, in my actual code i have another 8 of these and it covers two lines, i just simplified it a little to keep is simpler on stack. But when i have 12 conditions and they all need to be over 0 I was hoping for a more subtle method! – Oliver Klein Aug 07 '15 at 15:45
  • 1
    @OllieKlein Then you might be asking the wrong question. You might be better served by posting all the code involved in [codereview.se] or in another question on [so]. You would get better answers if we had the full picture. – isanae Aug 07 '15 at 17:56
  • @OllieKlein, if you have 12 conditions, and each of them needs to be calculated (and not just looked up in some database), you really need to consider your overall logic! You might need to change it into early returns, i.e. `cave = ... calculcation ...; if (cave <= 0) return; training = ... calculation ...; if (training <= 0) return;`. To calculate 12 conditions, and terminating on one failure, is a lot of wasted computing... – holroy Aug 07 '15 at 21:28

9 Answers9

46

You could get the lowest value with Math.min, and then you only need one check against the lower bound.

if(Math.min(cave, training, mobility, sleep) > 0) {
    //do something
}
RilezG
  • 608
  • 4
  • 6
  • 3
    Very clean and easy to understand. – rrowland Aug 06 '15 at 16:08
  • 83
    This is why simple, clever, concise and clean code is not necessarily good code. Code is about intent. If I run into this, I'll have to spend time trying to figure out 1) _what_ this code is doing, and 2) _why_ it is written like this. In the end, I'll be pissed off, I'll revert your changes and I'll make sure you get mandatory code reviews for the next year. – isanae Aug 06 '15 at 17:33
  • 11
    @RilezG We parse if statements with simple conditions all the time, every day. It takes little to no effort. This is not an idiom. It has to be analyzed, understood and learned. And for what? – isanae Aug 06 '15 at 17:49
  • 3
    @RilezG Though, if that is the case, the `0` value should be a constant/variable so you can change it in a single location then, and it's *clear* what the value means. – Der Kommissar Aug 06 '15 at 17:52
  • 7
    I see nothing wrong with your answer. Clever work indeed. Unreadable? A bit, but comparing to the ones using `bitwise XOR`, this is a piece of heaven – Ismael Miguel Aug 06 '15 at 18:11
  • I would comment it most definitely -- any non-obvious actions should be (and many obvious ones as well). – Jos Aug 06 '15 at 18:30
  • 5
    @Ismael The comparison should be to the original code, not to the worst of the suggestions for modification. – Kyle Strand Aug 06 '15 at 21:06
  • @KyleStrand Hum? What!? – Ismael Miguel Aug 06 '15 at 21:12
  • @Ismael The original code is quite readable. Why are you comparing this suggested answer to the other suggested answer, when the original is superior to both? – Kyle Strand Aug 06 '15 at 21:56
  • @KyleStrand How is it better? – Ismael Miguel Aug 06 '15 at 22:01
  • @IsmaelMiguel This suggestion involves using a math library for a simple logic-check. It is less clear, and it's very unlikely to be a worthwhile performance improvement (if indeed it improves the runtime efficiency at all). Also see isanae's comments. – Kyle Strand Aug 06 '15 at 22:13
  • @KyleStrand Basically: see again the reason why I praised this code? – Ismael Miguel Aug 06 '15 at 22:21
  • @Ismael Your original comment mentioned "bitwise or". That's the comparison to other, inferior suggestions that I'm criticizing. – Kyle Strand Aug 06 '15 at 22:33
  • @KyleStrand Quoting: "Also see isanae's comments" – Ismael Miguel Aug 06 '15 at 22:34
  • Let us [continue this discussion in chat](http://chat.stackoverflow.com/rooms/85356/discussion-between-kyle-strand-and-ismael-miguel). – Kyle Strand Aug 06 '15 at 22:42
  • +1 clever answer! everyone who thinks this is not readable: add a comment beforehand ffs! this is sweet and fast and simple, good job – Alex Aug 07 '15 at 12:34
34

You could use an array with .every. This is less DRY, but more verbose:

var isGreaterThanZero = function(val) {
    return val > 0;
};
if([cave, training, mobility, sleep].every(isGreaterThanZero)) {
    // Do Something
}

The reason I like this is that by using an array, it becomes apparent that you're repeating logic for every variable. Naming the callback in an obvious manner helps future readers understand exactly what that check will achieve. And finally, this gives scope for not just numbers, but any check of any type in the future - with any complexity hidden away from the if statement itself.

CodingIntrigue
  • 75,930
  • 30
  • 170
  • 176
  • you could also use `some` – Alex Aug 06 '15 at 12:28
  • All the arrays in if statement must be the same type right? `String` or `Integer` and so on. – Altay Mazlum Aug 06 '15 at 12:29
  • 1
    @Alex Polyfill is on the linked page. .@Altay Mazlum No. No type checking done here. That could be added in the helper function if required. – CodingIntrigue Aug 06 '15 at 12:31
  • This has the added benefit of permitting a function name with a more meaningful name, such as `statPositive`. It could even be modified slightly by wrapping the `val > 0` function inside of another function that would take a list: `if(allStatsPositive([cave, training, .... ]))` would be very clear. – Kyle Strand Aug 07 '15 at 17:25
29

As some have stated there is nothing wrong in having multiple simple conditions in an if statement, however I would consider changing the formatting into:

if ( cave > 0
   && training > 0
   && mobility > 0
   && sleep > 0 )

Alternatively I would change the from using these variables as integers, into bool variables, i.e. isCave, hasTraining, or similar, and then set the proper bool closer to where your code defines the different properties (Edit: And possibly return early if it is false, to prevent further unneccessary calculations). This would simplify your if statement to the latter in the next code block, which in addition shows a variant which can be used if the conditions becomes slightly more complex or you would like to ease the reading of the if statement:

var isCave =  cave > 0; # What does cave > 0 mean?
var hasTraining = training > 0;
var isMobile = mobility > 0;
var isNotSleeping = sleep > 0; # What does sleep > 0 indicate? Unclear

if (isCave && hasTraining && isMobile && isNotSleeping ) {
   // Do your thing
}

In other words, the multiple conditions in your if statement is not your biggest code smell, I would shift my focus to giving your variables better names clearly indicating what the value indicates. This would improve reading and understanding of your code, way more than some strange syntax to avoid multiple if conditions.

holroy
  • 3,047
  • 25
  • 41
8

There is nothing wrong with having multiple, simple conditions in an if statement. However, if it cannot fit into a single line (about 80 characters), you have a few solutions.

Ultimately, you are not checking whether four variables are greater than zero. You are checking for a set of conditions. The fact that these conditions are (currently) represented by signed integers is not only irrelevant, but is an implementation details that should be hidden away in functions.

  1. Use intermediary flags:

    var valid_location = false;
    if (cave > 0 && training > 0)
        valid_location = true;
    
    var valid_status = false;
    if (mobility > 0 && sleep > 0)
        valid_status = true;
    
    if (valid_location && valid_status)
        // ...
    
  2. Use a function:

    function can_do_this()
    {
        // split conditions into logical groups
    
        // checking location, because you need training if you're
        // in a cave
        if (cave <= 0 || training <= 0)
            return false;
    
        // checking status, because you have to be mobile and
        // sleepy
        if (mobility <= 0 || sleep <= 0)
            return false;
    
        return true;
    }
    
    if (can_do_this())
        // ...
    
  3. Use functions for the individual conditions you need to check:

    function valid_location()
    {
        return (cave > 0 && training > 0);
    }
    
    function valid_status()
    {
        return (mobility > 0 && sleep > 0);
    }
    
    if (valid_location() && valid_status())
        // ...
    
isanae
  • 3,253
  • 1
  • 22
  • 47
  • 1
    Please make that a simple `var valid_status = (mobility > 0 && sleep > 0);` – Bergi Aug 06 '15 at 21:22
  • @Bergi I had actually written something, which I edited out. I figured people would understand that a boolean expression could be used, unless it's too hard to read or it has too many conditions. Experienced programmers would know that, and I wouldn't want novices to think cramming all that in one line is a good thing. So no, I probably won't change it. – isanae Aug 06 '15 at 21:32
  • 1
    What do you mean by "cramming all that in one line"? Hardly matters whether the expression is in an assignment or an if condition, it's still "one line". Using `if`/`else` to set boolean variables is just a bad practise, especially for novice programmers. – Bergi Aug 06 '15 at 21:39
  • @Bergi This question is about having lots of flags to test. I don't like having lots of stuff on the right side of an assignment. I prefer having it in an `if` statement. I was afraid that by using an expression to initialize the flag, I would implicitly say that complex expressions on the right side is good. – isanae Aug 06 '15 at 21:52
  • 1
    I don't say that you should use a complex expression. I just say that when splitting them up (which is a proper solution to the problem), like you've done in your first snippet, you shouldn't put the parts in if statements that conditionally initialise helper flags - you should assign the helper flags directly. – Bergi Aug 06 '15 at 21:55
  • @Bergi I understand that you want me to initialize the flags directly instead of conditionally assigning them. I'm saying that I would agree with `var valid_status = (mobility > 0);`, but that I'd use an `if` for a more complex expression. – isanae Aug 06 '15 at 22:04
  • @isanae Novice programmers need to learn to see Boolean expressions as just another type of expression, not so different from operations on numbers or strings. They also need to learn not to hide side-effects like variable reassignment, if they can avoid it. In item 1, these concerns can be addressed by initializing the variables from Boolean expressions and never updating them afterward. – Keen Aug 07 '15 at 14:08
  • @isanae Overall, I approve of your recommendation that code like the OP's should be restructured to make the intent explicit. – Keen Aug 07 '15 at 14:10
4

Assuming 32-bit ints.

if ((cave | training | mobility | sleep) > 0)

If any of the above numbers are negative, the result of the OR will be negative and the requirements aren't met.

Edit: that's equivalent to if(cave >= 0 && training >= 0 && mobility >= 0 && sleep >= 0) and won't when some of the parameters are 0. The fix is to invert the sign bit:

if ((~cave | ~training | ~mobility | ~sleep) <= 0)

Some alternative ways which work even for floating-point values

if (cave*training*mobility*sleep > 0)
if (Math.sign(cave) * Math.sign(training) * Math.sign(mobility) * Math.sign(sleep) > 0)
if (Math.sign(cave) | Math.sign(training) | Math.sign(mobility) | Math.sign(sleep) > 0)
phuclv
  • 37,963
  • 15
  • 156
  • 475
  • You are right this is the right way to do it, and should be marked as the correct answer – oiledCode Aug 06 '15 at 13:00
  • Do note that this don't apply for any other case that is not `0`, clever work nevertheless – luiges90 Aug 06 '15 at 13:54
  • 4
    If `cave` equals `0` and the rest of the variables are positive, then the result of the `OR` statement will be positive. But this is not the desired result; it returns the opposite result from the OP's code. – mathmandan Aug 06 '15 at 14:11
  • 34
    [Write code for people first, machines second.](http://programmer.97things.oreilly.com/wiki/index.php/Write_Code_for_Humans_not_Machines) I guarantee I'd be okay with seeing `if(cave > 0 && training > 0 && mobility > 0 && sleep > 0)` in our codebase, but anything *clever* like `if ((cave | training | mobility | sleep) > 0)` will annoy me *because I won't know if it really does what you intended.* It doesn't matter that I can work out what it does if I can't tell what you meant. Save cleverness for solving problems, not creating them. – Keen Aug 06 '15 at 14:28
  • Surely in the second answer here, you just need to multiply them and check if it's bigger than 0? The first check is pretty redundant? – RemarkLima Aug 06 '15 at 14:40
  • 1
    @RemarkLima: If they are all negative then multiplying them will result in a non-zero and positive answer. – Chris Aug 06 '15 at 14:54
  • 5
    This isn't guaranteed to work with non integers. `0.2|0.5|0.6|0.9` is zero. – GOTO 0 Aug 06 '15 at 19:42
  • Note that bitwise operator in JS assumes 32-bit integer, so negative number larger than 32-bit will get truncated to the low 32-bit. As a result, the sign of the number now depends on whether the most significant bit is 0 or 1. For example, `-2340293804 | 5` returns a positive number, since -2340293804 MSB when truncated to 32-bit integer is 0. – nhahtdh Aug 07 '15 at 03:28
  • 1
    @Keen I'm not talking about readability here. The original way `if(cave > 0 && training > 0 && mobility > 0 && sleep > 0)` is clearly the easiest way to read, far more easier than any answers in this question. If the OP had asked for another way, I'd answer another possible (sometimes more efficient) way. He didn't say that it must be easier to read than his version, only easier to type and DRY. For DRY you must use a function which says it's meaning anyway. Some comments in the function may be needed – phuclv Aug 07 '15 at 04:32
  • 2
    @LưuVĩnhPhúc My previous comment is a warning to the OP (and to anyone else considering unreadable solutions). It is a warning against requesting code like this in the first place. Your answer was the type of thing requested, and it was an example of a brilliant solution that unfortunately misses edge cases and confuses readers, so I raised the concern here. Sorry I did not make it clear whom I was addressing. – Keen Aug 07 '15 at 14:01
4

Sounds like a job for a "validator" function like this:

function areAllGreaterThanZero(){
    //TODO: check inputs
    var result = true;
    [].splice.apply(arguments).forEach(function(x){ 
        result = result && (x > 0); 
    });
    return result;
}

if(areAllGreaterThanZero(cave, training, mobility, sleep)) {
    // ...
}
Alex
  • 23,004
  • 4
  • 39
  • 73
4

As others have suggested, you can use .every if you don't mind using ES6 or polyfills:

var hasAllStats = [cave, training, mobility, sleep]
  .every(function(stat) { return stat > 0; });

if (hasAllStats) { }

Alternatively, you can use .some to get the inverse (Also requires ES6 or polyfill):

var isMissingStats = [cave, training, mobility, sleep]
  .some(function(stat) { return stat <= 0; });

if (!isMissingStats) { }

If you don't want to use ES6, you can use reduce:

var hasAllStats = [cave, training, mobility, sleep]
  .reduce(function(hasAllStats, stat) {
    return hasAllStats && stat > 0;
  }, true);

if (hasAllStats) { }
rrowland
  • 2,734
  • 2
  • 17
  • 32
2

Filter it with lodash:

var data = [cave, training, mobility, sleep];
var result = _.filter(data, function (datum) { return datum > 0; }).length === data.length;

console.log(result);

It iterates over array elements and returns new array composed of those elements that meet given requirement of being > 0 - if result array is different size than given one it means one or more of it's elements were not > 0.

I wrote it specifically to check every array value (even if not necessary as first > 0 could give same result) to not stop on first positive as you stated you want to check all of them.

PS You can reverse it to check for <= 0 and check for .length === 0 instaed to be faster.

Szymon Toda
  • 4,454
  • 11
  • 43
  • 62
1

Why you are looking for solution?

Your question looks like best and simple answer, i recommend it. We have multiple solutions for this. Below one is that.

JSBin for .every()

Achieve it by using .every function

var flag = [cave, training, mobility, sleep].every(function(val) {
     return val > 0;
  });

if(flag) {
  alert('All the elements are greater than Zero');
}
Naveen Kumar Alone
  • 7,536
  • 5
  • 36
  • 57