Below a few ugly (I know, beauty lines in the eye of the beholder) lines of code I've seen lately in production. I also show alternatives which I find better. Maybe you find some of them useful, let me know.
# ugly
if (a > 42)
{
x <- b
}
else
{
x <- a
}
#---------------------------
# not so ugly:
x <- a
if (a > 42)
{
x <- b
}
If you call a function for which one argument depends on some condition, don't call the function twice, just define the argument based on the condition. The logic is then easier to follow. The examples here are simplified, in reality function calls can be quite complicated, do yourself a favour and simplify where you can.
# ugly
if (simple)
{
csv_load_result <- load_ssm_csv_data(
simple = simple,
path = simple_datapath
)
}
else
{
csv_load_result <- load_ssm_csv_data(
simple = simple,
path = complex_datapath
)
}
#---------------------------
# less ugly
path <- complex_datapath
if (simple)
{
path <- simple_datapath
}
csv_load_result <- load_ssm_csv_data(
simple = simple,
path = path
)
When calling functions: first define the arguments separately. It not only looks tidier, it also helps in debugging, so you can concentrate on a detail at a time.
And did I mention that, please please, don't align arguments and the equal sign - it might look esthetically to you, but it's just additional work to do the alignment the next time an argument name is modified or an argument is added to the function - I know there are automatic tools for this, but why would you spend time on this?
# definitely ugly
RES <- process_checks(
csv_table = csv_table,
RES = RES,
check_passed =
unlist(
lapply(
X =
lapply(
X = data_values,
FUN = grep,
x = valid_values_tmp
),
FUN = function(x){length(x)>0}
)
),
reject_reason = "value not found"
)
#---------------------------
# maybe not so ugly
`%>%` <- magrittr::`%>%`
check_passed <- lapply(
X = data_values,
FUN = grep,
x = valid_values_tmp
) `%>%`
lapply(
X = .,
FUN = function(x){length(x)>0}
) `%>%`
unlist(.)
RES <- process_checks(
csv_table = csv_table,
RES = RES,
check_passed = check_passed,
reject_reason = "value not found"
)
The one below is more of a koan. I find the second way of writing better, although I have problems with the name of the function, since 'between' does not tell me if the range is inclusive or not... So be careful when naming your functions...
# ugly
is_valid_code <- code >= 200 && code <= 299
# not so ugly
is_valid_code <- dplyr::between(x = code, left = 200, right = 299)
rlang is a gem, here is one example which helps in writing more expressive code.
# ugly
if (is.null(a)) {
x <- ""
}
# maybe not so ugly
`%||%` <- rlang::`%||%`
x <- a %||% ""
The one below made me take a deep breath. I have no idea why would I ever choose to use a switch on a logical converted to a character instead of a plain if statement...
# really ugly - why on earth...
mode <- switch(
as.character(is.null(id)),
"TRUE" = "insert",
"FALSE" = "update"
)
#---------------------------
# definitely not so ugly
mode -> "insert"
if (!is.null(id))
{
mode <- "update"
}
Make a promise. Show up. Do the work. Repeat.