Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Multi-argument function call as argument to another function #238

Open
david-romano opened this issue Dec 7, 2024 · 5 comments
Open

Multi-argument function call as argument to another function #238

david-romano opened this issue Dec 7, 2024 · 5 comments

Comments

@david-romano
Copy link

david-romano commented Dec 7, 2024

Here are two examples (based on first example from documentation for case_when()):

# Bad?
tibble(int = 1:70) |> 
  mutate(
    response = case_when(
      int %% 35 == 0 ~ "fizz buzz",
      int %% 5 == 0 ~ "fizz",
      int %% 7 == 0 ~ "buzz",
      .default = as.character(int)
    )
  )
# Good?
tibble(int = 1:70) |> 
  mutate(
    response = 
      case_when(
        int %% 35 == 0 ~ "fizz buzz",
        int %% 5 == 0 ~ "fizz",
        int %% 7 == 0 ~ "buzz",
        .default = as.character(int)
      )
  )

In the first case, the name responseobscures the fact that the arguments that follow belong to case_when(). This becomes much worse as the number of arguments and levels of nesting increase, so I would far prefer

    response = 
      case_when(

to

    response = case_when(

Thoughts? Could this be added to the guide?

@MichaelChirico
Copy link
Collaborator

I don't think the guide should weigh in on this. both are fine in different circumstances and I'm not sure it's worth defining detailed rules for which is which.

FWIW I typically prefer the first style.

@david-romano
Copy link
Author

FWIW I typically prefer the first style.

What about it do you prefer?

@MichaelChirico
Copy link
Collaborator

MichaelChirico commented Dec 8, 2024

adding vertical and horizontal whitespace does not come for free. it also presents tradeoffs. here I honestly don't see the confusion you do -- the case_when arguments are further indented than mutate().

this may not always be the case / can be up to taste. I personally don't think the style guide should weigh in. Evaluation can be done case-by-case between code author & reviewer.

@MichaelChirico
Copy link
Collaborator

BTW, I often choose to write code like this even more succinctly:

tibble(int = 1:70) |> 
  mutate(response = case_when(
    int %% 35 == 0 ~ "fizz buzz",
    int %% 5 == 0 ~ "fizz",
    int %% 7 == 0 ~ "buzz",
    .default = as.character(int)
  ))

The style is also guide-compliant and again, contextually preferable (or at least, unproblematic).

@luciorq
Copy link

luciorq commented Feb 9, 2025

BTW, I often choose to write code like this even more succinctly:

tibble(int = 1:70) |>
mutate(response = case_when(
int %% 35 == 0 ~ "fizz buzz",
int %% 5 == 0 ~ "fizz",
int %% 7 == 0 ~ "buzz",
.default = as.character(int)
))
The style is also guide-compliant and again, contextually preferable (or at least, unproblematic).

You mentioned that this formatting style is contextually dependent, and I agree entirely. However, I want to elaborate on the nuances.

In general, this succint style only works if the function (mutate() in this case) receives a single multi-line argument.
The moment you introduce additional required arguments, or if you need to create more than one column in a single mutate() call, this style breaks.

I also agree that for the specific example you showed, this style is perfectly fine and unproblematic.
Still, I believe its limitations should be more explicitly addressed in the style guide, as this pattern appears frequently in R and the Tidyverse. Below are some code examples that illustrate when this approach works well and when it becomes problematic:

Examples:

library(tidyverse)

# Ok and succinct
tibble(int = 1:70) |>
  mutate(response = case_when(
    int %% 35 == 0 ~ "fizz buzz",
    int %% 5 == 0 ~ "fizz",
    int %% 7 == 0 ~ "buzz",
    .default = as.character(int)
  ))

# Bad
tibble(int = 1:70) |>
  mutate(response = case_when(
    int %% 35 == 0 ~ "fizz buzz",
    int %% 5 == 0 ~ "fizz",
    int %% 7 == 0 ~ "buzz",
    .default = as.character(int)
  ), .before = int)


# Bad
tibble(int = 1:70) |>
  mutate(response = case_when(
    int %% 35 == 0 ~ "fizz buzz",
    int %% 5 == 0 ~ "fizz",
    int %% 7 == 0 ~ "buzz",
    .default = as.character(int)
  ),
    .before = int
  )


# Good
tibble(int = 1:70) |>
  mutate(
    response = case_when(
      int %% 35 == 0 ~ "fizz buzz",
      int %% 5 == 0 ~ "fizz",
      int %% 7 == 0 ~ "buzz",
      .default = as.character(int)
    ),
    sign = case_when(
      int > 0 ~ "positive",
      int < 0 ~ "negative",
      .default = "zero"
    ),
    .before = int
  )

Another example where this pattern appears and is the undocumented recommended (?) style:

# Good
testthat::test_that("Test context", {
  test_var <- 1 + 1
  expect_equal(test_var, 2)
})

In summary, the single-argument piping style can be concise and perfectly acceptable for simple cases, but it's important to recognize where it won't work. Making these caveats more explicit in the guide could help practitioners avoid confusion and use this stylistic choice wisely.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants