Take the 2-minute tour ×
Programmers Stack Exchange is a question and answer site for professional programmers interested in conceptual questions about software development. It's 100% free, no registration required.

In C and C++, it is very easy to write the following code with a serious error.

char responseChar = getchar();
int confirmExit = 'y' == tolower(responseChar);
if (confirmExit = 1)
{
    exit(0);
}

The error is that the if statement should have been:

if (confirmExit == 1)

As coded, it will exit every time, because the assignment of the confirmExit variable occurs, then confirmExit is used as the result of the expression.

Are there good ways to prevent this kind of error?

share|improve this question
30  
Yes. Turn on compiler warnings. Treat warnings as errors and it is not an issue. –  Loki Astari Aug 25 '12 at 6:39
    
possible duplicate of Doesn't "if (0 == value) ..." do more harm than good? –  Loki Astari Aug 25 '12 at 7:05
8  
In the given example, the solution is easy. You assign it a boolean value, thus use it as a boolean: if (confirmExit). –  Secure Aug 25 '12 at 7:16
2  
The problem is that the mistake was made by the C language "designers", when they chose to use = for the assignment operator and == for equality comparison. ALGOL used :=, because they specifically wanted to use = for equality comparison, and PASCAL and Ada followed the ALGOL decision. (It is worth noting that, when DoD solicited a C-based entry into the DoD1 bake-off, that eventually yielded Ada, Bell Labs declined, saying that "C was not now and would not ever be robust enough for DoD mission-critical software." One wishes that DoD and the contractors had listened to Bell Labs on this.) –  John R. Strohm Aug 25 '12 at 12:57
2  
@John, the choice of symbols isn't the problem, it's the fact that assignments are also an expression that returns the assigned value, allowing either a = b or a == b inside a conditional. –  Karl Bielefeldt Aug 25 '12 at 16:46

7 Answers 7

up vote 50 down vote accepted

The best technique is to increase the warning level of your compiler.
It will then warn you about potential assignment in the if conditional.

Make sure you compile your code with zero warnings (which you should be doing anyway). If you want to be pedantic then set your compiler to treat warnings as errors.

Using Yoda conditionals (putting the constant on the left hand side) was another technique that was popular about a decade ago. But they make the code harder to read (and thus maintain because of the unnatural why they read (unless you are Yoda)) and provide no greater benefit than increasing the warning level (which also has extra benefits of more warnings).

Warnings are really logical errors in the code and should be corrected.

share|improve this answer
2  
Who's Yoda ? I followed the links from your comment and no one explains this. Was (s)he a person ? Anyway, +1 :) . EDIT: Aaaaa, it was that stupid creature from Star Wars movies... as I didn't see them, I didn't know :D . –  Radu Murzea Aug 25 '12 at 8:25
1  
Definitely go with the warnings, not the Yoda conditionals - you can forget to do the conditional constant first as easily as you can forget to do ==. –  Michael Kohne Aug 25 '12 at 12:00
5  
I got a pleasant surprise that the most voted answer for this question is 'turn compiler warnings into errors'. I was expecting the if (0 == ret) horror. –  James Aug 25 '12 at 21:17
2  
@James: ... not to mention it will not work for a == b !! –  Emilio Garavaglia Aug 26 '12 at 21:30
3  
@EmilioGaravaglia: There's an easy workaround for that: Just write 0==a && 0==b || 1==a && 1==b || 2==a && 2==b || ... (repeat for all possible values). Don't forget the obligatory ...|| 22==a && 22==b || 23==a && 24==b || 25==a && 25==b ||... error, or the maintenance programmers won't have any fun. –  user281377 Nov 6 '12 at 12:16

You could always do something radical like testing your software. I don't even mean automated unit tests, just the tests every single experienced developer does out of habit by running his new code twice, once confirming the exit and once not. That's the reason most programmers consider it a non-issue.

share|improve this answer
1  
Easy to do when the behaviour of your program is completely deterministic. –  James Aug 25 '12 at 21:19
3  
This particular issue can be hard to test for. I've seen people bitten by rc=MethodThatRarelyFails(); if(rc = SUCCESS){ more than once, especially if the method fails only in conditions that are hard to test for. –  Steven Burnap Aug 25 '12 at 23:50
4  
That's because return codes are shitty, not because this is a problem. –  DeadMG Aug 26 '12 at 21:32
3  
@StevenBurnap: That's what mock objects are for. Proper testing includes testing of failure modes. –  Jan Hudec Nov 6 '12 at 13:47

A traditional way to prevent the incorrect use of assignments within expression is to place the constant on the left and the variable on the right.

if (confirmExit = 1)  // Unsafe

if (1=confirmExit)    // Safe and detected at compile time.

The compiler will report an error for the illegal assignment to a constant similar to the following.

.\confirmExit\main.cpp:15: error: C2106: '=' : left operand must be l-value

The revised if condition would be:

  if (1==confirmExit)    

As shown by comments below, this is considered by many to be an inappropriate method.

share|improve this answer
6  
11  
It should be noted that doing things this way will make you fairly unpopular. –  Stargazer712 Aug 25 '12 at 18:03
9  
Please don't recommend this practice. –  James Aug 25 '12 at 21:18
3  
It also doesn't work if you need to compare two variables to each other. –  dan04 Aug 27 '12 at 2:36
    
Some languages actually allow assigning 1 to a new value (yes, I know the Q is a bout c/c++) –  Matsemann Nov 7 '12 at 9:32

I agree with everyone saying "compiler warnings" but I want to add another technique: Code reviews. If you have a policy of reviewing all code that gets committed, preferably before it's committed, then it's likely this kind of thing will be caught during review.

share|improve this answer

I strongly agree that turning up your warning levels is also a good way to go, but I have worked with a lot of C and C++ programmers over the years, and have never heard that comparing the constant first if(1 == val) was a bad thing.

If your project leader approves of your doing this, don't worry about what other people think. The real proof is whether you or someone else can make sense of your code months and years from now.

share|improve this answer

Late to the party as ever, but Static Code Analysis is the key here

Most IDEs now provide SCA over and above the syntactic check of the compiler, and other tools are available, including those that implement the MISRA (*) and/or CERT-C guidelines.

Declaration: I am part of the MISRA C working group, but I'm posting in a personal capacity. I'm also independent of any tool vendor

share|improve this answer

Just use left hand assignment, compiler warnings can help but you have to make sure you get the right level otherwise you'll either be swamped with pointless warnings or won't be told ones you want to see.

share|improve this answer
    
You'd be surprised at how few pointless warnings modern compilers generate. You might not think a warning is important, but in most cases you're just wrong. –  Kristof Provost Nov 7 '12 at 12:54
    
Check out the book, "Writing Solid Code" amazon.com/Writing-Solid-Microsoft-Programming-Series/dp/…. It starts with a great discussion about compilers and how much benefit we could be getting from warning messages and even more extensive static analysis. –  DeveloperDon Nov 7 '12 at 14:43
    
@KristofProvost I've managed to get visual studio to give me 10 copies of the exact same warning fro the same line of code, then when this issue was 'fixed' it resulted in 10 identical warnings about the same line of code saying that the original method was better. –  Inverted Llama Nov 8 '12 at 11:09

Your Answer

 
discard

By posting your answer, you agree to the privacy policy and terms of service.

Not the answer you're looking for? Browse other questions tagged or ask your own question.