Most C++'ish way to check if value belongs to certain static set

Question

Let's say that I want to write something like this (the {1, 3, 7, 42, 69, 550123} set is known before compilation):

int x;
...
if (x == 1 || x == 3 || x == 7 || x == 42 || x == 69 || x == 5550123)
{
  ...
}

The condition looks ugly because we have 9 extra symbols ("|| x ==") for each possible value. How can I rewrite it in a more C++'ish way?

My best guess is:

int x;
...
const std::unordered_set<int> v = {1, 3, 7, 42, 69, 5550123};
if (v.count(x))
{
  ...
}

It has O(1) average complexity with some memory and time overhead, but still looks kinda ugly.

Answer 1

Here's my suggestion. It lets you write:

int x;

// ...

if ( x is_one_of {1, 3, 7, 42, 69, 550123} ) { do_stuff(); }

which is just about the most straightforward way you could think of.

... but - what dark magick be this? Surely I must be cheating, right?

Well, no, no cheating; or at least, not really cheating. This solution is the "most C++'-ish" in the sense that, employing multiple hacks including ugly ones, C++ lets you customize its effective syntax much more than you would expect!

Here's the implementation, in C++17:

#include <initializer_list>

namespace detail {

struct is_one_of_op {};

template <typename T>
struct is_one_of_op_primed { 
    T&& t; 

    template <typename... Us>
    constexpr bool operator+(std::initializer_list<Us...>&& list) {
        for(const auto& element : list) {
            if (t == element) { return true; }
        }
        return false;
    }
};

template <typename T>
constexpr is_one_of_op_primed<T> operator+(T&& t, is_one_of_op)
{
    return is_one_of_op_primed<T>{std::forward<T>(t)};
}

} // namespace detail

#define is_one_of + detail::is_one_of_op{} + std::initializer_list

See it in action: GodBolt

Notes:

• The inspiration for this implementation is partially due to this answer here on SO.
• Caveat: The values must all have the same type or bad things will happen >:-)

Answer 2

Try this:

#include <iostream>
#include <vector>
#include <algorithm>
int main()
{
  std::vector<int> v = {1, 3, 7, 42, 69, 5550123};
  auto is_present = [&v](int x)->bool{
      return  std::find(v.begin(),v.end(),x) != v.end();
  };      
  std::cout << (is_present(1)? "present" :" no present") <<std::endl;       
}

Answer 3

As the other answers say, move this to a function.

As the other answers say, you can consider adding constexpr / throw as required.

As the other answers don't say, use a switch case statement for this; which allows you to replace all the || x == with case - a few characters less, which might not seem significant (and it kinda isn't); but most importantly removes the chance of a mistake with either the variable name or a |.

When you've 300 items in this list, and it doesn't work as expected, trust me, you'll be glad to not have to check that every || is correct.

Answer 4

Edit: I just noticed c++14 tag. Note that my implementation of in relies on C++17. It can be done in C++14 as well using recursion, but that involves much more boilerplate, and a bit slower compilation.

One could use a template to generate a function with a logical operator sequence, such as the one in nvoigt's answer:

template<auto... ts, class T>
constexpr bool
in(const T& t) noexcept(noexcept(((t == ts) || ...))) {
    return ((t == ts) || ...);
}

// usage
if (in<1, 3, 7, 42, 69, 5550123>(x))

That said, hiding the set of magic numbers behind a named function probably makes a lot of sense:

constexpr bool
is_magical(int x) noexcept {
    return in<1, 3, 7, 42, 69, 5550123>(x);
}

Answer 5

How can I rewrite it in a more C++ way?

Suggestion: make a variadic template function for it, make it generic (non only for int values) and make it constexpr; this way you can check, the presence of the value in the set, compile time.

You tagged C++14 but I show first the recursive way for C++11, with a recursive case and a ground case

template <typename T>
constexpr bool is_in_list_11 (T const &)
 { return false; }

template <typename T, T t0, T ... ts>
constexpr bool is_in_list_11 (T const & t)
 { return (t == t0) || is_in_list_11<T, ts...>(t); }

Starting from C++14, a constexpr function can be a lot more complex, so recursion isn't necessary anymore and you can write something as

template <typename T, T ... ts>
constexpr auto is_in_list_14 (T const & t)
 {
   using unused = bool[];

   bool ret { false };

   (void)unused { false, ret |= t == ts ... };

   return ret;
 }

Starting from C++17 you can use also auto template type and template folding, so (as user2079303 showed before) the function become very very simple

template <auto ... ts, typename T>
constexpr auto is_in_list_17 (T const & t)
 { return ( (t == ts) || ... ); }

The following is a full working example with all versions

#include <iostream>

template <typename T>
constexpr bool is_in_list_11 (T const &)
 { return false; }

template <typename T, T t0, T ... ts>
constexpr bool is_in_list_11 (T const & t)
 { return (t == t0) || is_in_list_11<T, ts...>(t); }

template <typename T, T ... ts>
constexpr auto is_in_list_14 (T const & t)
 {
   using unused = bool[];

   bool ret { false };

   (void)unused { false, ret |= t == ts ... };

   return ret;
 }

template <auto ... ts, typename T>
constexpr auto is_in_list_17 (T const & t)
 { return ( (t == ts) || ... ); }

int main ()
 {
   constexpr auto b11a { is_in_list_11<int, 1, 3, 7, 42, 69, 5550123>(7) };
   constexpr auto b11b { is_in_list_11<int, 1, 3, 7, 42, 69, 5550123>(8) };

   constexpr auto b14a { is_in_list_14<int, 1, 3, 7, 42, 69, 5550123>(7) };
   constexpr auto b14b { is_in_list_14<int, 1, 3, 7, 42, 69, 5550123>(8) };

   constexpr auto b17a { is_in_list_17<1, 3, 7, 42, 69, 5550123>(7) };
   constexpr auto b17b { is_in_list_17<1, 3, 7, 42, 69, 5550123>(8) };

   std::cout << b11a << ' ' << b11b << std::endl;
   std::cout << b14a << ' ' << b14b << std::endl;
   std::cout << b17a << ' ' << b17b << std::endl;
 }

Answer 6

The only clean way to do this is to simply move it to a method. Name the method appropriately and it really does not matter what you do inside.

bool is_valid_foo_number(int x)
{
    return x == 1
        || x == 3
        || x == 7
        || x == 42
        || x == 69
        || x == 5550123;
}

The method looks good enough for me, because all I will ever see of it is

if (is_valid_foo_number(input))
{
    // ...
}

Should a technical detail change (like the sheer amount of valid numbers requiring another lookup approach or maybe a database instead of hardcoded values) you change the method's internals.

The point is that I think it only looks ugly... because you need to look at it while you look at your logic. You shouldn't have to look at the details anyway.