Your immediate problem is that you are using =
(assignment) rather the ==
(equality):
if(ch=='.' || ch=='"' || ch=='(' || ch=')' || ...
___^____
see here
What's happening is to do with the relative precedence of the various bits. Although ==
will bind at a higher level than ||
, a == b || c == d
evaluates as (a == b) || (c == d)
.
However, =
does not bind at a higher level so a == b || c = d
evaluates as ((a == b) || c) = d)
. With your code, that ends up with a part expression of:
('(' || ch) = ')'
In other words, you're trying to assign ')'
to the non-lvalue '(' || ch
.
You actually got lucky there by not parenthesising every term (which many people do) since that would make it syntactically correct but not do what you expect at runtime:
if ((ch == '.') || (ch == '"') || (ch == '(') || (ch = ')') || ...
_________^^^^^^^^^^_________
will assign rather than test
The usual approach to avoid that is to put the constant first but, like you, I believe that's just plain ugly:
if (('.' == ch) || ('"' == ch) || ('(' == ch) || (')' = ch) || ...
^^^^^^^^
/ urk! \
When I'm doing that sort of thing, I tend to rely on the standard library so that my code looks better and has much less chance of being incorrect:
if (strchr (".\"()!,?;", ch) != NULL) {
// It's valid.
}
or even better:
#define VALID_PUNCTUATION ".\"()!,?;"
if (strchr (VALID_PUNCTUATION, ch) != NULL) {
// It's valid.
}
or even better than that, put the whole segment in a function and just use something like:
if (isValidPunctuation (ch)) {
// It's valid.
}
(you'll still have to make sure the code in the function is correct but your mainline code will be much cleaner).