How to shorten multiple && condition

Question

Currently I can run my program but my code consists of a lot of repetition which looks something like:

while(option != 'E' && option != 'D' && option != 'P' && option != 'Q' &&
      option != 'e' && option != 'd' && option != 'p' && option != 'q') {
  // Some code here
}

or:

while(cType != 'S' && cType != 'L' && cType != 'O' && cType != 'Q' &&
      cType != 's' && cType != 'l' && cType != 'o' && cType != 'q') {
  // Some code here
}

What's the fastest way to shorten above code?

(Is there any way beside using additional function?)

Case-convert to lower (or upper) and reduce the number of comparisons needed? — Jonathan Leffler, Aug 24 '15 at 18:37
You can get rid of half the conditions with [`std::tolower`](http://en.cppreference.com/w/cpp/string/byte/tolower) — NathanOliver, Aug 24 '15 at 18:38
I would recommend putting the logic in a function and call the function. — R Sahu, Aug 24 '15 at 18:39
Completely depends on the use case. And pick a language please. — Lightness Races in Orbit, Aug 24 '15 at 18:42
[Maybe you shouldn't](http://stackoverflow.com/q/26124620/17034). — Hans Passant, Aug 24 '15 at 18:43
Have an unsigned char array where a lookup array[unsigned] provides the desired result — , Aug 24 '15 at 18:47
@Vui La Chinh it has to be only letters, how about something like this ==>> https://stackoverflow.com/questions/31770861/validate-parameter-for-0-or-1/31772378#31772378 ? — Michi, Aug 24 '15 at 19:01
What's the significance of the note _(is there any way beside using additional function?)_ in the question? If you can't use any other functions, then there isn't much that'll speed that up. You might guess that users are likely to type lower case letters rather than upper case, and test the lower case before the upper case. You might prioritize the options in the most probable to least probable sequence. But that's all micro-optimization compared with case-conversion (and you don't do that manually). — Jonathan Leffler, Aug 24 '15 at 19:30
Are you approaching the stage where a lookup is going to be more effective than strings of boolean expressions? — Martin James, Aug 24 '15 at 19:37

asdf · Answer 1 · 2015-08-24T19:21:18.483

8

const char invalidChars[] = "edpq";
while (strchr(invalidChars, tolower(option)) != 0) {
    ...
}

edited Aug 24 '15 at 19:21

answered Aug 24 '15 at 18:40

asdf

2,927
2
21
42

Not my downvote, but: you seem to be dereferencing `option` which is a plain `char` in the question, not a pointer (_now fixed_). You've also inverted the meaning of the characters; I believe they're the valid options. You should note that your solution is appropriate for C (the question is tagged with C and C++). You should consider: `const char validOptions[] = "edpq"; while (strchr(validOptions, tolower(option)) != 0)` with half as much string to search. – Jonathan Leffler Aug 24 '15 at 19:11
@JonathanLeffler My only concern was that the `tolower` method would actually be more costly than checking both upper and lower options. – asdf Aug 24 '15 at 19:13
Measurement is the only way to resolve that. `tolower()` usually maps to a reference to a global array. Your string search is using a string constant, which is also a global array. There's not a huge amount to choose between them, but search 4 characters instead of 8 should be some marginal benefit. That said, you'd be hard-pressed to measure, much less notice, the difference. – Jonathan Leffler Aug 24 '15 at 19:15
@JonathanLeffler My concern would be that the global array `tolower` needs to search through would need to be 26 chars long while the upper and lower array in this case is only 8. I haven't run any benchmarks on this, but given my understanding of c and its underlying functions I could imagine this would be the case. – asdf Aug 24 '15 at 19:18
3

No; the global array would be 256 bytes long and would be accessed directly by `tolower(option)` with `global_array[(option)]` (more or less — there are some tricks to do with EOF, etc so it would probably be 257 bytes and `global_array[(option)+1]`), but the gist is pretty much the same. No searching — just a direct array access. – Jonathan Leffler Aug 24 '15 at 19:19
@JonathanLeffler Ah you're correct. Using ASCII codes as the index could solve that issue. I'll update my answer to reflect this. Thank you. – asdf Aug 24 '15 at 19:21
2

You may have used `tolower` incorrectly; if `option` is a `char` then it should be `tolower((unsigned char)option)`. I'd suggest just having the upper case characters in the match string and not using tolower at all – M.M Aug 24 '15 at 21:14
@MattMcNabb It is correct in the scope that I understand. The poster's code does not extend far enough for me to know specifically what `option` is coming in as. However, this is a question about optimization and using `tolower` will be the fastest way to handle it given the parameters specified. – asdf Aug 24 '15 at 21:24
While using the same literals, the condition is the complement of the first in the question. – greybeard Oct 25 '15 at 03:09

score 3 · Answer 2 · answered Aug 24 '15 at 18:42

3

You could initialize a string containing the characters you want to match, then use find, which returns npos if no match was found:

string match = "SLOQsloq";
while (match.find(cType) == string::npos)
  ...

answered Aug 24 '15 at 18:42

Jon Purdy

53,300
8
96
166

score 3 · Answer 3 · edited Aug 24 '15 at 19:06

3

You could get rid of half of the conditions with std::tolower

while(std::tolower(option) != 'e' && std::tolower(option) != 'd' && std::tolower(option) != 'p' && std::tolower(option) != 'q')

You could also use a std::string and it's find member function like:

std::string options = "edpq";
//...
while (options.find(std::tolower(option)) == std::string::npos)

edited Aug 24 '15 at 19:06

Jonathan Leffler

730,956
141
904
1,278

answered Aug 24 '15 at 18:46

NathanOliver

171,901
28
288
402

2

Not my downvote, but calling `tolower()` once might improve things... but calling it for each comparison may be *less* efficient. And the string option is neater-looking than the original, but probably slower. – Dmitri Aug 24 '15 at 18:51
Why not: `std::string options = "edpq"; while (options.find(std:tolower(option)) == std::string::npso)` with half as much string to search? – Jonathan Leffler Aug 24 '15 at 19:02
@JonathanLeffler It didn't even cross my mind to do that. I changed the code example. Thank you. – NathanOliver Aug 24 '15 at 19:05

score 2 · Answer 4 · answered Aug 24 '15 at 18:41

You could simplify the logic and make things more readable by using an std::set and checking if the set contains (or doesn't contain) the variable we're comparing to:

std::set<char> someChars { 'a', 'b', 'c' };
if(someChars.find(myChar) != someChars.end()) {
    // myChar is either 'a', 'b', or 'c'
}

The condition in most other languages would be written more cleanly as something like someChars.contains(myChar) (but C++'s set interface is very minimal).

However, for a small number of comparisons, your method is probably faster.

score 1 · Answer 5 · answered Sep 02 '15 at 20:02

A candidate quick test with no branching.

1) Make case-less.
2) Form product of 4 differences.
3) 1 compare against 0.

Assumes only letters.
Works up to 4 (sizeof int/sizeof char) letters.
Upper/Lower case differ by the same bit. (This works with ASCII and EBCDIC)

#define CaseMaskBits ((unsigned char)~('A'^'a'))
#define Product4(ch, s) ((ch-s[0]) * (ch-s[1]) * (ch-s[2]) * (ch-s[3]))
#define TestEq4(ch, t, s)  (t=ch&CaseMaskBits, !Product4(t, s))

int main(void) {
  int ch;
  printf("%X\n", CaseMaskBits);
  for (ch = 0; ch < 256; ch++){
    int t;  // temp var for TestEQ4
    while (TestEq4(ch, t, "ELPQ")) {
      printf("%d %c\n", ch, ch);
      break;
    }
  }
  return 0;
}

280           while (TestEq4(ch, t, "ELPQ")) {
00402560:   mov %ebx,%eax
00402562:   and $0xdf,%eax
00402567:   lea -0x4c(%eax),%edx
0040256a:   lea -0x45(%eax),%ecx
0040256d:   imul %edx,%ecx
00402570:   lea -0x50(%eax),%edx
00402573:   sub $0x51,%eax
00402576:   imul %ecx,%edx
00402579:   imul %edx,%eax
0040257c:   test %eax,%eax
0040257e:   jne 0x402555 <main+37>
281             printf("%d %c\n", ch, ch);

score 0 · Answer 6 · edited May 23 '17 at 12:23

If option/cType have <= 8 bits of significance, for speed, use a table lookup. @Dieter Lücking

unsigned char Table[UCHAR_MAX + 1] = {
  fill per needs };
#define Table_OptionMask 1
#define Table_cTypeMask 2
#define Table_nextMask 4

while (!(Table[(unsigned char)option] & Table_OptionMask)) ...
while (!(Table[(unsigned char)cType] & Table_cTypeMask)) ...

For simpler code maintenance, populate the table on code startup by calling Table_Setup().

static void Table_SetInsensitive(unsigned char *Table, unsigned mask, cnst char *src) {
  while (*src) {
    Table[toupper((unsigned char) *src)] |= mask;
    Table[tolower((unsigned char) *src)] |= mask;
    src++;  
  }
}

void Table_Setup(void) {
  memset(Table, 0, sizeof Table);
  Table_SetInsensitive(Table, Table_OptionMask, "EDPQ");
  Table_SetInsensitive(Table, Table_cTypeMask, "SLOQ");
  Table_SetInsensitive(Table, Table_cTypeMask, tbd);
}

How to shorten multiple && condition

6 Answers6