469,289 Members | 2,225 Online
Bytes | Developer Community
New Post

Home Posts Topics Members FAQ

Post your question to a community of 469,289 developers. It's quick & easy.

Assignments in conditions should be avoided

code green
1,726 Expert 1GB
I have just switched from phpdesigner to Netbeans as my script editor.
Netbeans is criticising my code with this warning
Possible accidental assignment, assignments in conditions should be avoided
What is your opinion of the warning when doing this for example?
Expand|Select|Wrap|Line Numbers
  1.  if($data = $file($filepath))
The alternative is
Expand|Select|Wrap|Line Numbers
  1. $data = $file($filepath);
  2. if($data)
which seems less professional.
Oct 29 '09 #1

✓ answered by Markus

@code green
This is discouraged in most languages and mainly because of readability, I think. TBH, I could glance over that code and think that what I saw was a comparison check. To avoid this ambiguity, I'd do it like:

Expand|Select|Wrap|Line Numbers
  1. if (($data = somefunc()) != 0) { ...
  2.  

16 9698
Dheeraj Joshi
1,123 Expert 1GB
@code green
I don't know why it complains but.....

My organization insist me to write code in this way.
Expand|Select|Wrap|Line Numbers
  1. $data = $file($filepath);
  2. if($data)
  3.  
If i club more statements in a condition like

Expand|Select|Wrap|Line Numbers
  1. if($data = $file($filepath))
  2.  
i will get review comments.

Regards
Dheeraj Joshi
Oct 29 '09 #2
Dheeraj Joshi
1,123 Expert 1GB
I dont know about that problem, but my organization has some coding standards, as per that.

If i try to write code as

Expand|Select|Wrap|Line Numbers
  1. if($data = $file($filepath))
  2.  
my managers gives review comments to this code saying it to change as

Expand|Select|Wrap|Line Numbers
  1. $data = $file($filepath);
  2. if($data)
  3.  
Regards
Dheeraj Joshi

Regards
Dheeraj Joshi
Oct 29 '09 #3
code green
1,726 Expert 1GB
I have had a Google and it seems the reasoning behind this is to avoid the schoolboy error of confusing
Expand|Select|Wrap|Line Numbers
  1. if($data = $file($filepath)) 
and
Expand|Select|Wrap|Line Numbers
  1. if($data == $file($filepath)) 
Are script editors now assuming the lowest common denomitor of programming skill?
Oct 29 '09 #4
Dormilich
8,651 Expert Mod 8TB
donít know about thatÖ but admittedly, if someone other than you looks at the statement
Expand|Select|Wrap|Line Numbers
  1. if ($var = function($param))
(s)he might not be sure, whether it is intended or a mistake.
Oct 29 '09 #5
Markus
6,050 Expert 4TB
@code green
This is discouraged in most languages and mainly because of readability, I think. TBH, I could glance over that code and think that what I saw was a comparison check. To avoid this ambiguity, I'd do it like:

Expand|Select|Wrap|Line Numbers
  1. if (($data = somefunc()) != 0) { ...
  2.  
Oct 29 '09 #6
code green
1,726 Expert 1GB
So it seems to be discouraged for maintainability.
Hmm, I have never made the comparator/assignment error, even when reading.
But I like to comply with best practice protocol and would hate future coders criticising my legacy code.
I quite like Markus' method.
Maybe I should listen to Netbeans and modify where required
Oct 29 '09 #7
Markus
6,050 Expert 4TB
I wonder how the following code is evaluated: if ($data = somefunc())?

Is the value of somefunc() assigned to $data and then $data's value is checked, or is the assignment checked (does the assignment fail)?

I'm going to ask on the PHP dev mailing list.
Oct 29 '09 #8
Dormilich
8,651 Expert Mod 8TB
I donít think an assignment can failÖ itís usually expressions that fail.
Oct 29 '09 #9
code green
1,726 Expert 1GB
I wonder how the following code is evaluated: if ($data = somefunc())
Logic tells me that the most inner bracket command is carried out first,
then working outwards.
So the return value of somefunc() is assigned to $data even if this value is FALSE.
Then the if() condition is evaluated.

If PHP was strictly typed then $data could not be a boolean or say an array.
So does loosely typed allow potential bugs to creep in
Oct 29 '09 #10
Markus
6,050 Expert 4TB
@Dormilich
An assignment is an expression?
Oct 29 '09 #11
Markus
6,050 Expert 4TB
You're correct, I think. The end value of the expression is used. You see, this is where the ambiguity comes in. If I write it as if (($data = somefunc())), then it makes perfect sense that the result from the innermost expression is used.

/doh.
Oct 29 '09 #12
Dormilich
8,651 Expert Mod 8TB
@Markus
is my English that bad?
Oct 29 '09 #13
Markus
6,050 Expert 4TB
@Dormilich
No. My understanding is that an assignment is an expression.

Edit: I guess not.
Oct 29 '09 #14
code green
1,726 Expert 1GB
A note to Netbeans users, the warning can be disabled in
Tools>>Options then Hint tab. Select PHP for Language.
Uncheck the "Possible accidental assignment..." option under Stable.

This of course does not mean it is acceptable practice.

I checked the "Experimental" option under hints which revealed a lot more sins in my code.
Mainly unitialised variables, but I can live with those.
Oct 29 '09 #15
Markus
6,050 Expert 4TB
Here is a good reply that I got:

Assignment operations in PHP have the "side effect" of returning the
assignment. For example:

function return_false() {
return false;
}

var_dump(return_false()); //bool(false);
var_dump($a = return_false()); //bool(false);
var_dump($a = 1); // int(1)
var_dump($a = "hello world!"); //string...

So the same thing that allows you to do:

$a = $b = $c = $d = 154;

which works because "$d = 154" returns 154, which is assigned to $c,
which returns 154... is how assignment in conditionals or looping
works:
if($a = return_false()) { }
var_dump($a); //bool(false)

if($a = "hello") {}
var_dump($a); //string, "hello"

So what's really happening is the return value of the expression "$a =
____" is evaluated and that's used to determine the truth of the
conditionality. if($a = return_false()) is exactly the same thing as
if(return_false()) save for you "capture" the output of the function,
rather than just allow the conditional operator to see it. It's
functionally equivalent to $a = return_false(); if($a) {} but it's
important to understand that __assigning a variable to a value in PHP
is an expression with a return value___ and that return value is the
value that you assigned to the variable.
Oct 29 '09 #16
code green
1,726 Expert 1GB
As I come from a C++ background,
in my own functions if I wish to return a value and/or an error code,
I prefer to pass a pointer to the function and return the error code
Expand|Select|Wrap|Line Numbers
  1. $data = array();
  2. if(getData($data)){....}
  3.  
  4. function getData(&$retData){
  5.     $success = false;
  6.     ...
  7.    //load data into $retData
  8.   //If successful then $success = true;
  9.    ...
  10.     return $success;
  11. }  
But PHP inbuilt functions don't do it this way.
Very few even accept a pointer as a parameter.
Oct 29 '09 #17

Post your reply

Sign in to post your reply or Sign up for a free account.

Similar topics

4 posts views Thread by Jacek Generowicz | last post: by
9 posts views Thread by Mark Twombley | last post: by
3 posts views Thread by bearophileHUGS | last post: by
21 posts views Thread by Paul Steckler | last post: by
8 posts views Thread by Fredrik Tolf | last post: by
reply views Thread by Simon Burton | last post: by
17 posts views Thread by Brian Blais | last post: by
1 post views Thread by CARIGAR | last post: by
reply views Thread by suresh191 | last post: by
By using this site, you agree to our Privacy Policy and Terms of Use.