Don't assign from an if statement

[DavisVaughan]

cond <- TRUE

# do this
if (cond) {
  x <- "this"
} else {
  x <- "that"
}

# not this
x <- if (cond) {
  "this"
} else {
  "that"
}

The second method is altogether more difficult to read

• x <- if (cond) { is a really ugly line
• each condition just holds a "floating" result: "this" and "that". You have to mentally parse that one of these is then assigned back to x

Note that for the return value of a function I do use the fact that the if statement returns the evaluated expression

get_result <- function(cond) {
  if (cond) {
    "this"
  }
  else {
    "that"
  }
}

[jennybc]

Ooh I'm interested to see where this discussion goes, because I'm not sure I agree. Especially in the special case where you can make it very compact:

x <- if (cond) "this" else "that"

But, yeah, there is definitely some limit.

Related point: I suspect that assigning via an if() condition is always a bad idea.

# please no
if (m <- f(x, y, z)) {
  # lots of code involving m, x, y, z
} else {
  # lots of other code involving m, x, y, z
}

[DavisVaughan]

Going from single line to multiline is actually exactly where it changed so maybe that should be relaxed a bit r-lib/vctrs#329 (comment)

[jennybc]

# "internal" assignment   # "external" assignment
if (cond) {               x <- if (cond) {
  x <- "this"               "this"
} else {                  } else {
  x <- "that"               "that"
}                         }

The external assignment style seems to signal that the only thing going on in if (cond) {...} else {...} is about determining x. And if x is not your focus today, you don't have to read it. It suggests there are no side effects.

Whereas internal assignment makes it harder to tell that these blocks are just about x. Side effects seem more likely, so you have to read it to find out.

[hadley]

I can't quite figure out what the general principle actually is — you can't say that an assignment should always fit on a line because that would prohibit assigning from a pipe. Maybe this is a fairly specific combination of conditional assignment?

Maybe it's not the <- that this is really about, but the action of the contents of the if block?

... reading @jennybc's comment ...

Yeah, in the external assignment form, you can't easily tell the purpose of one of the if blocks just by reading it. You also have to read outwards to understand that the point of the block is to create a new variable. And that violates the general principle that "less context to understand code is better"

[hadley]

Maybe the pattern is "keep side-effects local".

[jennybc]

Here's a real-world example I just found in reprex:

  txt <- if (requireNamespace("devtools", quietly = TRUE)) {
    "devtools::session_info()"
  } else {
    "sessionInfo()"
  }

https://github.com/tidyverse/reprex/blob/916aff0b4665ae7758c4d7c89b77d1b017f87fa6/R/whisker.R#L69-L73

[jimhester]

The big benefit to using foo <- if(y) 1 else 2 over if (y) foo <- 1 else foo <- 2 is removing the replication of the assignment, you can accidentally forget to update one of the two clauses, or have a typo and assign to the wrong variable etc.

However I think most of the time when you want to reach for foo <- if (... it is clearer to put the logic in a helper function, e.g. in jenny's example above.

session_info <- function() {
  if (requireNamespace("devtools", quietly = TRUE)) {
    "devtools::session_info()"
  } else {
    "sessionInfo()"
  }
}

txt <- session_info()

[DavisVaughan]

I just found myself wanting to do:

  predictions <- switch(
    type,
    response = predict_model_response(model, predictors),
    other = predict_model_other(model, predictors)
  )

this is slightly different because you can't assign inside the switch statement, but should the switch statement be encapsulated in its own function? something like:

predictions <- predict_switch(type, model, predictors)

[jennybc]

Or what about:

predfun <- switch(type, response = predict_model_response, other = predict_model_other)
predictions <- predfun(model, predictors)

[DavisVaughan]

I do like the separation since they both take the same arguments. But I assume we'd get into the same issue if a third or fourth prediction type was added, where it would have to be a multiline switch statement to be < 80 characters in width.

[hadley]

I don't mind from a switch statement, even if it's multiline.