If then assignment

[billfor]

I am preparing for a code review. Our code reviews are very thorough. I need an oppinion on
something. Should this first code block labeled (Before be rewritten into the second code
block labeled After: ???

In general I think we try to avoid more efficent and less lines of code. So I don't know wether
the After code block is better?? Also, is it okay to assign a value to the variable Flag at line
5 in the after code block. The value is being assigned in a if statement, and I do not know if this
is okay.

statement.


Before :

1 bool SomeClass::SomeMethod(___)
2 {
3 boolean Flag=false;
4 Flag = myObeject.myMethod( inParam);
5
6 if(!Flag)
7 {
8 FunctionGetSomeData(pIndex,Dvar),
9 FunctionWriteData(Dvar);
10 }
11
12 /* about 25 more lines of code below this */
.
37 return Flag;
38}

After :

1 bool SomeClass::SomeMethod(___)
2 {
3 boolean Flag;
4
5 if(!(Flag = myObeject.myMethod( inParam)))
6 {
7 FunctionGetSomeData(pIndex,Dvar),
8 FunctionWriteData(Dvar);
9 }
10 /* about 25 more lines of code below this */
.
35 return flag
36 }

[willchop]

You might consider not even bothering with the flag
variable.

bool SomeClass::SomeMethod(___)
{
if (!myObeject.myMethod( inParam))
{
FunctionGetSomeData(pIndex,Dvar),
FunctionWriteData(Dvar);
return false;
}

return true;
}


regards, willchop

[jfaust]

The first one is better, but you might as well put it on the same line:

1 bool SomeClass::SomeMethod(___)
2 {
3          boolean Flag = myObeject.myMethod( inParam);
4
5          if(!Flag)
6          {
7	      FunctionGetSomeData(pIndex, Dvar),
8	      FunctionWriteData(Dvar);
9          }
11
12        /* about 25 more lines of code below this */
.
37        return Flag;
38}

Your second example leaves the Flag variable uninitialized, which should be avoided. Also, the name Flag does not mean anything.

Jeff

[villemos]

Originally posted by billfor
1 bool SomeClass::SomeMethod(___)
2 {
3 boolean Flag;
4
5 if(!(Flag = myObeject.myMethod( inParam)))
6 {
7 FunctionGetSomeData(pIndex,Dvar),
8 FunctionWriteData(Dvar);
9 }
10 /* about 25 more lines of code below this */
.
35 return flag
36 }

Or, if the flag is used later in the "about 25 more lines of code below this", use the copy constructor. Else don't bother about flag;

1 bool SomeClass::SomeMethod(___)
2 {
3 boolean Flag(myObeject.myMethod(inParam));
4
5 if(Flag == false)
6 {
7 FunctionGetSomeData(pIndex,Dvar);
8 FunctionWriteData(Dvar);
9 }
10 /* about 25 more lines of code below this */

Has the benefit of not initailising flag first and then assigning.

By the way; I prefere always to compare for example "flag" to something (i.e. flag == false instead of !flag). Any oppinions anyone?

villemos.

[jfaust]

I prefer "if( flag )", for absolutely no reason. I just think it reads better. "If OK then ..." sounds better than "If OK is true then". It's somewhat redundant.

Jeff

[villemos]

Fair enough

Anyone with an efficiency/safety reason?

villemos

[PaulWendt]

I want all of my if conditionals to be boolean expressions. If the
variable being tested returns a bool, then I will use only that.
If I'm comparing a pointer for nullness, however, I will do an
explicit comparison against 0 ... which may seem like overkill, but
I feel things are more consistent that way.

bool flag = true;

if (flag)
{
   // do whatever
}


SomeClass* p = dynamic_cast<SomeClass*>(somePointer);
if (p != 0) // instead of if (p)
{
   // work with p here
}

--Paul

[willchop]

The alternative example I posted earlier may be more efficient
than the others, although I'm not sure. The other examples
are well enough, but do they introduce an additional assignment
(the flag variable) - or is that assignment normally optimized
by the compiler? I would think that in my example the compiler
would create a temporary return value whose scope would cease
immediately after the boolean evaluation. But if a "real" variable
is present to hold the return value, does the temporary still get
created and then get copied to the "real" flag variable? Are these
details compiler specific?

Anyone have some insight on this?

regards, willchop

[vinodp]

----------------------------Reply to------------------
1 bool SomeClass::SomeMethod(___)
2 {
3 boolean Flag(myObeject.myMethod(inParam));
4
5 if(Flag == false)
6 {
7 FunctionGetSomeData(pIndex,Dvar);
8 FunctionWriteData(Dvar);
9 }
10 /* about 25 more lines of code below this */

Has the benefit of not initailising flag first and then assigning.

By the way; I prefere always to compare for example "flag" to something (i.e. flag == false instead of !flag). Any oppinions anyone?
---------------------------------------------------------------
First of all boolean is not a standard C++ data type (see standard 3.9.1) Its MS extension

so for boolean its always good to explicitaly check with false value bcoz boolean may contain value other than true or false.

In case of bool i think it doesn't matter as C++ standard says that check 3.9.1 point 6
"Values of type bool are either true or false"

Vinod

[Axter]

I agree with willchop, and that you should remove the flag if you're worried about efficiency.
I also agree with one of the other experts in that you should do a test for true, instead of false.
Test for true are easier to read, and less prone to mistakes then test for false.

With that in mind, I recommend the following code:

bool SomeClass::SomeMethod(___)
{
  if (myObeject.myMethod( inParam)) return true;
  FunctionGetSomeData(pIndex,Dvar),
  FunctionWriteData(Dvar);
  return false;
}

I think the above code is more compact, and easier to read.

[vinodp]

code:--------------------------------------------------------------------------------
bool SomeClass::SomeMethod(___)
{
if (myObeject.myMethod( inParam)) return true;
FunctionGetSomeData(pIndex,Dvar),
FunctionWriteData(Dvar);
return false;
}
--------------------------------------------------------------------------------
Axter, i think its always good to have single return point in function
bcoz its easy to maintain the function & less chances of resource leak
So from maintainace point of view i am not agree with ur implementation

Vinod

[willchop]

If readability is a concern I would additionally suggest
returning an enumerated status value (eGood, eBad)
instead of a boolean value. Often there is confusion and
conflicts between functions and the meaning of their
boolean result

This code is both effiicent and readable:

EStatus SomeClass::SomeMethod(___)
{
if (myObeject.myMethod( inParam) == eGood)
return eGood;

FunctionGetSomeData(pIndex,Dvar),
FunctionWriteData(Dvar);
return eBad;
}

regards, willchop

[willchop]

Originally posted by vinodp
Axter, i think its always good to have single return point in function
bcoz its easy to maintain the function & less chances of resource leak
So from maintainace point of view i am not agree with ur implementation

vinodp,
I partially agree with you. Although, sometimes keeping the
return status in scope until the end of a function can also be
a maintenance problem (status flags tend to get recklessly
re-used by programmers who aren't familiar with the code).
Also, keeping track of which resources to release at the end
of the function according to the state the function left them in
at the point of the error/end-of-control can also be
maintenance/readiblity/operational problem.

regards, willchop

[Axter]

Axter, i think its always good to have single return point in function
bcoz its easy to maintain the function & less chances of resource leak
So from maintainace point of view i am not agree with ur implementation

I disagree. It depends on the code. If you have a large amount of code in a function, then I agree, that it's better to have one exit point.
But when you have a small function, the function ends up looking more complicated then it has to be when it's forced to have only one exit.
When possible, it's best to keep it simple. Keeping it simple makes it more readable and easier to maintain.

[villemos]

I think billfor needs to clarify something;
Does the 25 lines of code which comes after the initial Flag check use Flag? If not, then I agree with Axter and willchop's suggestions. Else the suggestions may need some more work. Anyway, chances are, as it is a boolean flag, that you no matter what can leave it out completly, i.e. do as Axter and willchop suggest below in the 25 lines of code as well if Flag is used.

I'm not sure about the single exit point, even in short functions, and most functions I try to keep short and to the point.

I prefere functions to return a default value, i.e. the most "normal" condition. Whats normal depends on the function and please I don't wan't to get into a long filosophical discussion of "normallity". I know its subjective. Unormal conditions that should break of the operation returns in the function after handling their condition. I find that else the code tends to become something like

STATUS
A::function doStuff()
{
STATUS status (GOOD);
// Do Stuff A
if(status)
{
// DoStuff B
if(status)
{
status = GOOD;
}
else
{
status = BAD;
}
}
else
{
status = BAD;
}

// Do Stuff C

return status;
}

And here I have been nice and only included two status checks instead of the 10 that will surely be present somewhere in one of your functions I'm not sure is more readable or maintaniable than

STATUS
A::function doStuff()
{
STATUS status (GOOD);
// Do Stuff A
if (status == BAD)
{
return status ;
}

// Do Stuff B
if (status == BAD)
{
return status;
}

// Do Stuff C

return status;
}

Or in operators; Is (I left out the {} as my company only have a limited budget of brackets)

int
operator==(const Object& obj) const
{
boolean Flag(false);
if (obj.A == this->A)
if (obj.B == this->B)
if (obj.C == this->C)
flag = true;

return Flag;
}

More readable or maintaniable than

int
operator==(const Object& obj) const
{
if (obj.A == this->A)
if (obj.B == this->B)
if (obj.C == this->C)
return true;

return false;
}

But I agree on the principle; Keep it simple and if possible have only one return point. Functions with 10+ if statements is likely overcomplicated anyway.

villemos