Is it good practice to ever cast a boolean to an integer for arithmetic?

Question

If I have a function where I am operating on a specific offset within a string, and the exact location of this offset depends on a previous test within the function, is it "bad practice" or "ugly" to use the boolean in the arithmetic in which the offset is calculated?

Here's my function:

#include <Windows.h>
#include <strsafe.h>

VOID WINAPI TrimQuotes(WCHAR *wStr, INT cchStr, WCHAR *wOut, INT cchOut)
{
    WCHAR wStart = wStr[0];
    WCHAR wEnd = wStr[cchStr-1];

    if(wStart == L'\"')
        StringCchCopy(wOut, cchOut, wStr+1);
    else
        StringCchCopy(wOut, cchOut, wStr);
    if(wEnd == L'\"')
        wOut[cchStr-(1+(wStart==L'\"'))]=L'\0';
}

As you probably guessed from its name, its job is to determine if a string is surrounded by double-quote characters and if so, remove them from the string. If I removed a character from the source string already in the case that wStart == L'\"', then the offset within the destination string where the last non-null character is decremented by one, so my solution was to simply cast the boolean into an integer that would adjust the offset from cchStr if necessary:

Would you recommend doing this?

Answer 1

There's a practical difference between using a well-known, well-understood coding technique, and an exotic, ad-hoc one that will need to be analyzed by other programmers to figure out what it does.

For example

while (i) { 

Everyone who is a C programmer with a little experience understands how this works. When i reaches zero, the loop exits.

Your technique? Not so much.

If I were you, I would favor code clarity over cleverness.

Answer 2

In your original, you use a local variable to extract the first character, then use that local twice but in the same exact expression. Since you've already tested that once, we don't need the two boolean comparisons, one of which is the subject of the question and is indeed potentially confusing.

Since it can easily be eliminated, why not do that?

Further, you are not handling empty strings.

I think the following is more readable: the main logic employs ...CopyN and is not conditional. Only the adjustments for the starting and end position require conditional logic. Each character comparison condition is tested only once.

VOID WINAPI TrimQuotes ( WCHAR *wStr, INT cchStr, WCHAR *wOut, INT cchOut )
{
    if ( cchStr > 0 && wStr[cchStr-1] == L'\"' )
        cchStr--;

    if ( cchStr > 0 && wStr[0] == L'\"' ) {
        wStr++;
        cchStr--;
    }

    StringCchCopyN ( wOut, cchOut, wStr, cchStr );
}

Answer 3

Casting a boolean to integers can sometimes be an elegant solution, e.g. when trying to avoid branches as an ultra-low-level optimization, or when trying to prevent timing attacks.

For anything else, it is best to strive for the most readable, most maintainable code. Clever code is dangerous code, because we might miss a bug. Is it obvious what that last statement does? Not really.

Note also that your weird control flow will remove the first leading quote and the last trailing quote even when the string is not enclosed in quotes:

"foo   → foo
""foo  → "foo
foo"   → foo
foo""  → foo"
"foo"  → foo
""foo" → "foo
...

Is that really the intended behaviour? If you only want to remove enclosing quotes, I'd expect code like this: (pseudocode)

TrimQuotes(in, out):
  if in[0] == '"' and in[-1] == '"':
    copy in[1, ..., -2] into out
  else:
    copy in into out

If you wanted to remove a leading and trailing quote even if they don't match, I'd expect something like this:

TrimQuotes(in, out):
  first = 0
  last = length of in - 1
  if in[first] == '"':
    first++
  if in[last] == '"':
    last--
  copy in[first, ..., last] into out

If you want to remove any number of leading and trailing quotes, I would expect:

TrimQuotes(in,  out):
  first = 0
  last = length of in - 1
  while in[first] == '"':
    first++
  while in[last] == '"':
    last--
  copy in[first, ..., last] into out

Any of these shows more clearly why you are moving indices in what direction. Calculating all indices before you do the copy also prevents you from copying more than necessary. In your code, you still copy the last character even if it will be erased.

Another curious point is that your function performs a copy. Trimming the string could be done entirely by incrementing the pointer and decrementing the character count (as demonstrated in Erik Eidt's answer). If the caller would require a modifiable copy, they could make one themselves.