4

we had a programing lesson today. Very easy exercise in console. I wrote a loop to load from console char by char by getchar() with assignment, all of these in loop term.

char c;
while((c = getchar()) != '\n'){
...

Someone says, that this isn't safe to use, others says, that in C/C++ I can do this, but not in C#.

I tried this

string s;
if((s = Console.ReadLine()) != ""){
...

But this also works, so I don't understand why this is unsafe. Or isn't it?

Edit:\ I also read this Why would you use an assignment in a condition? but this isn't answer to my question at all.

Community
  • 1
  • 1
Arxeiss
  • 976
  • 16
  • 34
  • In the second case should it be "\n" or "" is correct ? – Omkant Oct 24 '12 at 12:40
  • Those are not equivalent statements. – asawyer Oct 24 '12 at 12:40
  • Why do you have doubts that this can be unsafe? – besworland Oct 24 '12 at 12:42
  • No, because C# doesn't include "\n" in string which is returned. But C++ does. But this isn't important, important is if I can use assingment in term of loops or if statements. – Arxeiss Oct 24 '12 at 12:44
  • 1
    Please expand upon what "safe" really means here. It's impossible to give an answer unless we know what the *exact* concerns are. Unsafe to you might not be a concern to others (for example, I have no problem doing assignments in conditional checks, I'm comfortable with it, it's safe because *I* understand it, but others might feel differently). – casperOne Oct 24 '12 at 12:47
  • @casperOne His first example doesn't work, and can result in an endless loop. "unsafe" is, in this case, an euphemism for broken. – James Kanze Oct 24 '12 at 12:50
  • 1
    `Someone says, that this isn't safe to use` Did you ask "Someone" why they felt it wasn't "safe"? – Mike Oct 24 '12 at 12:52
  • 5
    And what sort of a git closes this? He posts invalid code, and asks why it is invalid. I can't think of a more legitimate question (even if he's somewhat vague about why he isn't sure of the code). – James Kanze Oct 24 '12 at 12:52
  • @JamesKanze From the close reason box *immediately below the comments* (emphasis mine): "It's difficult to tell what is being asked here. This question is ambiguous, **vague**, incomplete..." By your own words, this should be closed. When that vagueness is removed, then it's appropriate to reopen the question. – casperOne Oct 24 '12 at 12:57
  • @casperOne I do not think the question is vague. It seems very specific to me. Please explain what you think is wrong with the question in more detail. In what way is it vague? – ctype.h Oct 26 '12 at 17:36
  • @ctype.h Have read [my previous comment](http://stackoverflow.com/questions/13049369/is-it-safe-to-use-assignment-in-condition-c-c-c-sharp#comment17719716_13049369)? It's pretty clear. – casperOne Oct 26 '12 at 17:37
  • @ctype.h As well as the comment to James. – casperOne Oct 26 '12 at 17:38
  • I don't think, that my codes, first or second, are invalid. Both works as I expected, but some people said, that this isn't great, because of some memory alocation etc... or just said: "You cannot use assingment in condition". – Arxeiss Oct 27 '12 at 14:25

2 Answers2

3

One can argue about the readability of such code (and it wouldn't pass code review at most places I've worked), but the problem isn't the fact that there is an assignment. The problem is the fact that getchar() doesn't return a char, it returns an int. And that the set of possible return values won't fit in a char. If you change your code to:

int c;
while ( (c = getchar()) != EOF && c != '\n' ) {
...

it would be "safe" (but I still wouldn't want to maintain it). If you do want the update of c in the loop control, use a for loop:

for ( int c = getchar(); c != EOF && c != '\n'; c = getchar() ) {
...

This is at least readable.

James Kanze
  • 150,581
  • 18
  • 184
  • 329
  • `wouldn't pass code review at most places I've worked` the assignment in a condition, or the fact that the return of `getchar()` is being stored into a char? I'd be surprised if it was the former, seems to be to be a fairly common practice. – Mike Oct 24 '12 at 12:53
  • @Mike Both, but what I was thinking of was the assignment in a conditional. Single statements that do more than one thing generally make code difficult to read. (On the other hand, the a lot of places would make an exception to side effects in a conditional for the single case of IO, and things like `while ( std::getline(...) )` are widespread and idiomatic, despite the hidden assignment.) – James Kanze Oct 25 '12 at 07:23
  • The `for` loop version is illegal in C. I would prefer the `while` loop version. – ctype.h Oct 26 '12 at 17:52
2

The main operation in your sample is != which is not an assignment. What you can not do in C# (And I think it was the right design decision) is something like this:

if (s = "")
...

The problem here is that it is very similar to the usual equals operator ==. There are cases when this code is intentional, but usually it is a typo which is very difficult to find. Compare it with this:

if (s == "")
...

When you are looking for a bug in your code, you can easily overlook this.

ctype.h
  • 1,470
  • 4
  • 20
  • 34
Ondra
  • 1,619
  • 13
  • 27
  • He's asking about safety, not readability. The unsafe aspect of his code (and in this case, "unsafe" means that it doesn't work correctly for certain inputs, and in fact results in a endless loop) is the fact that he assigns an `int` to a `char`, when certain values the `int` can take won't fit in a `char`. – James Kanze Oct 24 '12 at 12:47
  • Fortunately, modern c++ compilers would emit a warning when encoutering `if (s = "")` and suggest `if ((s = ""))`. – arnoo Oct 24 '12 at 12:50