Assign variable in if condition statement, good practice or not?

I moved one years ago from classic OO languages such like Java to JavaScript. The following code is definitely not recommended (or even not correct) in Java:

if(dayNumber = getClickedDayNumber(dayInfo))
{
    alert("day number found : " + dayNumber);
}
function getClickedDayNumber(dayInfo)
{
    dayNumber = dayInfo.indexOf("fc-day");
    if(dayNumber != -1)	//substring found
    {
	    //normally any calendar month consists of "40" days, so this will definitely pick up its day number.
	    return parseInt(dayInfo.substring(dayNumber+6, dayNumber+8));
    }
    return false;
}

Basically I just found out that I can assign a variable to a value in an if condition statement, and immediately check the assigned value as if it is boolean.

For a safer bet, I usually separate that into two lines of code, assign first then check the variable, but now that I found this, I am just wondering whether is it good practice or not in the eyes of experienced JavaScript developers?

I wouldn't recommend it. The problem is, it looks like a common error where you try to compare values, but use a single = instead of == or ===. For example, when you see this:

if (value = someFunction()) {
    ...
}

you don't know if that's what they meant to do, or if they intended to write this:

if (value == someFunction()) {
    ...
}

If you really want to do the assignment in place, I would recommend doing an explicit comparison as well:

if ((value = someFunction()) === <whatever truthy value you are expecting>) {
    ...
}

I see no proof that it is not good practice. Yes, it may look like a mistake but that is easily remedied by judicious commenting. Take for instance:

if (x = processorIntensiveFunction()) { // declaration inside if intended
    alert(x);
}

Why should that function be allowed to run a 2nd time with:

alert(processorIntensiveFunction());

Because the first version LOOKS bad? I cannot agree with that logic.

I did it many times. To bypass the JavaScript warning, I add two parens:

if ((result = get_something())) { }

You should avoid it, if you really want to use it, write a comment above it saying what you are doing.

There is one case when you do it, with while-loops.
When reading files, you usualy do like this:

void readFile(String pathToFile) {
    // Create a FileInputStream object
    FileInputStream fileIn = null;
    try {
        // Create the FileInputStream
        fileIn = new FileInputStream(pathToFile);
        // Create a variable to store the current line's text in
        String currentLine;
        // While the file has lines left, read the next line,
        // store it in the variable and do whatever is in the loop
        while((currentLine = in.readLine()) != null) {
            // Print out the current line in the console
            // (you can do whatever you want with the line. this is just an example)
            System.out.println(currentLine);
        }
    } catch(IOException e) {
        // Handle exception
    } finally {
        try {
            // Close the FileInputStream
            fileIn.close();
        } catch(IOException e) {
            // Handle exception
        }
    }
}

Look at the while-loop at line 9. There, a new line is read and stored in a variable, and then the content of the loop is ran. I know this isn't an if-statement, but I guess a while loop can be included in your question as well.

The reason to this is that when using a FileInputStream, every time you call FileInputStream.readLine(), it reads the next line in the file, so if you would have called it from the loop with just fileIn.readLine() != null without assigning the variable, instead of calling (currentLine = fileIn.readLine()) != null, and then called it from inside of the loop too, you would only get every second line.

Hope you understand, and good luck!

If you were to refer to Martin Fowlers book Refactoring improving the design of existing code ! Then there are several cases where it would be good practice eg. long complex conditionals to use a function or method call to assert your case:

> "Motivation > > One of the most common areas of complexity in a program lies in complex conditional logic. > As you write code to test conditions and to do various things depending on various > conditions, you quickly end up with a pretty long method. Length of a method is in itself a factor that makes it harder to read, but conditions increase the difficulty. The problem > usually lies in the fact that the code, both in the condition checks and in the actions, > tells you what happens but can easily obscure why it happens. > > As with any large block of code, you can make your intention clearer by decomposing it and > replacing chunks of code with a method call named after the intention of that block of code. > With conditions you can receive further benefit by doing this for the conditional part and > each of the alternatives. This way you highlight the condition and make it clearly what you > are branching on. You also highlight the reason for the branching."

And yes his answer is also valid for Java implementations. It does not assign the conditional function to a variable though in the examples.

You can do this in Java too. And no, it's not a good practice. :)

(And use the === in Javascript for typed equality. Read Crockford's The Good Parts book on JS.)

I came here from golang, where it's common to see something like

if (err := doSomething(); err != nil) {
    return nil, err
}

In which err is scoped to that if block only. As such, here's what I'm doing in es6, which seems pretty ugly, but doesn't make my rather strict eslint rules whinge, and achieves the same.

{
  const err = doSomething()
  if (err != null) {
    return (null, err)
  }
}

The extra braces define a new, uh, "lexical scope"? Which means I can use const, and err isn't available to the outer block.

You can do assignments within if statements in Java as well. A good example would be reading something in and writing it out:

http://www.exampledepot.com/egs/java.io/CopyFile.html?l=new

The code:

// Copies src file to dst file.
// If the dst file does not exist, it is created
void copy(File src, File dst) throws IOException 
{
    InputStream in = new FileInputStream(src);
    OutputStream out = new FileOutputStream(dst);

    // Transfer bytes from in to out
    byte[] buf = new byte[1024];
    int len;
    while ((len = in.read(buf)) > 0) {
        out.write(buf, 0, len);
    }
    in.close();
    out.close();
}

It's not good practice. You soon will get confused about it. It looks similiar to a common error: misuse "=" and "==" operators.

You should break it into 2 lines of codes. It not only helps to make the code clearer, but also easy to refactor in the future. Imagine that you change the IF condition? You may accidently remove the line and your variable no longer get the value assigned to it.

you could do something like so:

if (value = /* sic */ some_function()){
  use_value(value)
}