better way to write this if condition?

[cloudEndeavor]

like the title says and any resource on reducing repetitive code is appreciated.

  if ( n1 > 0 && n1 < 1001 && n2 > 0 && n2 < 1001 && n3 > 0 && n3 < 1001 )
{}

[kigar64551]

I think there is no redundancy here, as all three variables are checked to be within the range from 1 (inclusive) to 1000 (inclusive). This requires 6 comparisons in total, i.e. two comparisons per variable: one comparisons to check the lower bound of the variable, and then another comparisons to check its upper bound.

Still, you could rewrite the code using pre-processor macros:

#define CHECK_RANGE(X) (((X) > 0) && ((X) < 1001))

void foo(void)
{
    if (CHECK_RANGE(n1) && CHECK_RANGE(n2) && CHECK_RANGE(n3))
    {
        /* ... */
    }
}

Note: After macro expansion, this code will be no different from your original code. So it only makes to code "look" cleaner (more readable), but it does not reduce the number of comparisons.

______

Of course, you also could write an actual function to check the range for each variable:

int check_range(const int x)
{
    return (x > 0) && (x < 1001);
}

void foo(void)
{
    if (check_range(n1) && check_range(n2) && check_range(n3))
    {
        /* ... */
    }
}

[George P]

@kigar, IN_RAGE? o_O

Don'cha mean IN_RANGE since that is what your macro is named.

[kigar64551]

Fixed 😂

[George P]

A rather crude C++ way to do range checking by creating a function that returns a bool:

#include <iostream>

bool IN_RANGE(const int);

int main()
{
   int n1 { 10 };
   int n2 { 20 };
   int n3 { 30 };

   if ( IN_RANGE(n1) && IN_RANGE(n2) && IN_RANGE(n3) )
   {
      std::cout << "Every variable between 0 & 1001...\n";
   }
   else
   {
      std::cout << "Something is out of range...\n";
   }
}

bool IN_RANGE(const int X)
{
   return (X > 0 && X < 1001 );
}

Every variable between 0 & 1001...

Change one of the variables to a value out of the 1 - 1000 range, say int n1 { -10 }; and get this output:

Something is out of range...

Dang! The preview window is now ALL FOOKED UP!

[Cubbi]

stdlib may not have IN_RAGE, but boost does:

#include <boost/numeric/interval.hpp>
namespace bn = boost::numeric;
int main()
{
    auto i = bn::interval(1, 1000);
    if ( in(n1, i) && in(n2, i) && in(n3, i) )
...

[jonnin]

you can make it shorter and less easy to understand, eg
if(n1*n2*n3 && ...) //cleans up the zero checks
or split it up:
if(n1 && n1 <1001)
if(n2...

but no matter what you do here, you are just making the c++ pretty. Most of your choices will have no real impact in making it 'better' in terms of function, and what you have is fairly readable
though you may consider breaking it up a bit visually.

[lastchance]

if ( min( { n1, n2, n3 } ) > 0 && max( { n1, n2, n3 } ) < 1001 )

#include <iostream>
#include <algorithm>
using namespace std;

int main()
{
   int n1 = 12, n2 = 1, n3 = 1000;
   if ( min( { n1, n2, n3 } ) > 0 && max( { n1, n2, n3 } ) < 1001 ) cout << "Good!\n";
   else                                                             cout << "Bad!\n";
}

[seeplus]

#include <iostream>
#include <algorithm>
#include <initializer_list>
#include <concepts>

template<typename T> requires std::integral<T>
bool check_range(std::initializer_list<T> il,  T mn, T mx) {
	return std::ranges::all_of(il, [&](T i) {return i >= mn && i <= mx; });
}

int main() {
	const int n1 { 12 }, n2 { 1 }, n3 { 1000 };

	if (check_range({n1, n2, n3}, 0, 1001))
		std::cout << "Good!\n";
	else
		std::cout << "Bad!\n";
}

[jonnin]

that is pretty. But does it do more work? I can't tell easily. Adding function overheads or checking everything before stopping or whatnot makes it easy to accidentally get the answer with several extra steps, just to rearrange the code a little (again, the original was not awesome but not that bad either). Like the min/max version is super clean but it for sure has 1 extra comparison I believe at best, and 3 extra (because lack of short circuit) at worst (??) if I did that right in my head.

[lastchance]

jonnin wrote:

Like the min/max version is super clean but it for sure has 1 extra comparison I believe at best, and 3 extra (because lack of short circuit) at worst (??) if I did that right in my head.

The min/max version has six comparisons (exactly the same as the others): 2 implicit ones in each of min and max and 2 in the comparisons with 0 and 1001.

It has a single && operation (as compared to 5 in the original).

But I doubt that any of the codes took any measurable amount of time.

Its main deficiency over an in_range() function is that the range might have been different for each variable - in which case it wouldn't have worked.

[jonnin]

yes, I guess I was saying the min max have to do all the checks, while the straight up version can stop if one is out of bounds directly.
No, its not important generally, unless the fixed comparison is in a gigantic loop (clearly not, here).

Right, its not general purpose. Thats ok if you think its unlikely to change, its tighter than the GP ones.

[seeplus]

while the straight up version can stop if one is out of bounds directly.

The std::all_of() version does stop when one is out of bounds with false.

[lastchance]

jonnin wrote:

yes, I guess I was saying the min max have to do all the checks

No, the min() case is addressed first, and if that fails then the code doesn't have to do the max().

The problem looks like one for which it is going to have to do all the checks most of the time.

[jonnin]

yes, I see, you are right!

[SubZeroWins]

if ((n1 > 0) && (n1 < 1001) &&
(n2 > 0) && (n2 < 1001) &&
(n3 > 0) && (n3 < 1001))
{}

Rearrange to look cleaner.

In a check such as (x >= 1) vs x > 0, even though the former has short circuiting it is probably more efficient to implement the latter?
With (x >= 1) it checks x>1 before x=1?

How do you insert left-side spaces on this forum without it left shifting the empty spaces?

[jonnin]

With (x >= 1) it checks x>1 before x=1?

with optimizations on, it is likely smart enough to do 1 check here, eg < 2

short circuiting correctly is difficult. You honestly need to know what case is most likely to fail and check that first, and this can be consistent or random or vary from user to user etc. Hardware helps with branch prediction somewhat anyway, but that is more limited than people like to admit: it does not do that much for a complex condition with lots of terms that can't be optimized to just a few terms.

you can't do spacing without using a tag. The output, code, and other tags let you have spaces and indentation for code or even just to make a point. Regular post text does not have it.

[George P]

How do you insert left-side spaces on this forum without it left shifting the empty spaces?

Using code tags would be helpful: http://www.cplusplus.com/articles/jEywvCM9/

   if ( (n1 > 0) && (n1 < 1001) &&
        (n2 > 0) && (n2 < 1001) &&
        (n3 > 0) && (n3 < 1001) )
   { }

There are other tags available: http://www.cplusplus.com/articles/z13hAqkS/, cplusplus uses a subset of BBCode tags.

[seeplus]

With (x >= 1) it checks x>1 before x=1?

The compiler will generate code to compare x with 1 and then one instruction for the branch. Something like:

cmp eax, 1
bge L1

It doesn't first check for greater-than and then check for equal.

The cmp sets the flags accordingly and then the following branch instruction checks these flags and branches accordingly depending upon the set flags and the particular branch instruction used.