Upgrade to Pro — share decks privately, control downloads, hide ads and more …

Code Smells and Feels

Code Smells and Feels

Jennifer (Jenny) Bryan

July 13, 2018
Tweet

More Decks by Jennifer (Jenny) Bryan

Other Decks in Programming

Transcript

  1.  @jennybc
     @JennyBryan
    Jennifer Bryan 

    RStudio
    Code Smells and Feels
    rstd.io/code-smells

    View full-size slide

  2. Slides not meant to stand alone!
    See full references here:
    rstd.io/code-smells
    which currently points to
    https://github.com/jennybc/code-smells-and-feels#readme

    View full-size slide

  3. German & Econ major
    Management Consultant
    (Bio)statistics PhD → Prof
    Software Engineer

    View full-size slide

  4. Workflow: You should have one
    Zen And The aRt Of Workflow Maintenance
    Code Smell and Feels

    View full-size slide

  5. Your taste develops faster than your ability.
    via boredpanda, bbc, reddit

    View full-size slide

  6. Why does my code not smell like theirs?

    View full-size slide

  7. structures in code that suggest
    (or scream for)
    refactoring
    code smell

    View full-size slide

  8. make code
    - easier to understand
    - cheaper to modify
    without changing behaviour
    refactor

    View full-size slide

  9. "Never use attach()."
    "Always put a space before and after =."
    "Have better taste."
    "Write more elegant code."

    View full-size slide

  10. "Never use attach()."
    "Always put a space before and after =."
    "Have better taste."
    "Write more elegant code."
    Detect a code smell.
    Apply the prescribed refactoring.

    View full-size slide

  11. code smell or ...?
    romance novel
    legal instrument
    quilt pattern
    movie franchise
    Prince song

    View full-size slide

  12. Too Hot to Handle
    Primitive Obsession
    Forbidden Fruit Tree
    When Doves Cry
    Inappropriate Intimacy
    Fast and Furious
    Restraining Order
    Middle Man
    code smell
    romance novel
    legal instrument
    quilt pattern
    Prince song
    movie franchise
    ???

    View full-size slide

  13. Too Hot to Handle
    Primitive Obsession
    Forbidden Fruit Tree
    When Doves Cry
    Inappropriate Intimacy
    Fast and Furious
    Restraining Order
    Middle Man
    romance novel
    code smell
    quilt pattern
    Prince song
    code smell
    movie franchise
    legal instrument
    code smell

    View full-size slide

  14. Too Hot to Handle
    Forbidden Fruit Tree
    When Doves Cry
    Fast and Furious
    Restraining Order
    new code smell?
    new code smell?
    new code smell?
    new code smell?
    new code smell?

    View full-size slide

  15. Duplicated Code
    Long Method
    Large Class
    Long Parameter List
    Divergent Change
    Shotgun Surgery
    Feature Envy
    Data Clumps
    Primitive Obsession
    Switch Statements
    Parallel Inheritance Hierarchies
    Lazy Class
    Speculative Generality
    Temporary Field
    Message Chains
    Middle Man
    Inappropriate Intimacy
    Alt. Classes w/ Diff. Interfaces
    Incomplete Library Class
    Data Class
    Refused Bequest
    Comments

    View full-size slide

  16. Refactoring, Chapter 9:
    Simplifying Conditional Expressions

    View full-size slide

  17. rstd.io/code-smells
    all code snippets are here:

    View full-size slide

  18. x bizarro(x)
    -77, 0, 4 77, 0, -4
    TRUE, FALSE FALSE, TRUE
    "desserts", "god" "stressed", "dog"

    View full-size slide

  19. x <- 1:5
    #x <- c(TRUE, FALSE, FALSE, TRUE, FALSE)
    cat(
    "The bizarro version of x is",
    -x,
    #!x,
    "\n"
    )
    #> The bizarro version of x is -1 -2 -3 -4 -5

    View full-size slide

  20. #x <- 1:5
    x <- c(TRUE, FALSE, FALSE, TRUE, FALSE)
    cat(
    "The bizarro version of x is",
    #-x,
    !x,
    "\n"
    )
    #> The bizarro version of x is FALSE TRUE TRUE FALSE TRUE

    View full-size slide

  21. Tip #1:
    Do not comment and uncomment sections of
    code to alter behaviour.

    View full-size slide

  22. Tip #1:
    Do not comment and uncomment sections of
    code to alter behaviour.
    Tip #1a:
    Especially not in multiple places that you will
    never, ever keep track of

    View full-size slide

  23. x <- 1:5
    #x <- c(TRUE, FALSE, FALSE, TRUE, FALSE)
    cat(
    "The bizarro version of x is",
    if (is.numeric(x)) {
    -x
    } else {
    !x
    },
    "\n"
    )
    #> The bizarro version of x is -1 -2 -3 -4 -5

    View full-size slide

  24. Tip #2:
    Use if () else () in moderation.

    View full-size slide

  25. Tip #2:
    Use if () else () in moderation.
    Tip #2a:
    Describe in grandiose terms:
    "I used a one layer neural network with identity
    activation and no hidden layers." -- Federico Vaggi

    View full-size slide

  26. bizarro <- function(x) {
    if (is.numeric(x)) {
    -x
    } else {
    !x
    }
    }
    bizarro(1:5)
    #> [1] -1 -2 -3 -4 -5
    bizarro(c(TRUE, FALSE, FALSE, TRUE, FALSE))
    #> [1] FALSE TRUE TRUE FALSE TRUE

    View full-size slide

  27. Tip #3:
    Use functions.

    View full-size slide

  28. Tip #3:
    Use functions.
    Tip #3a:
    A few little functions >> a monster function.

    View full-size slide

  29. Tip #3:
    Use functions.
    Tip #3a:
    A few little functions >> a monster function.
    Tip #3b:
    Small well-named helper >> commented code

    View full-size slide

  30. bizarro <- function(x) {
    if (class(x)[[1]] == "numeric" || class(x)[[1]] == "integer") {
    -x
    } else if (class(x)[[1]] == "logical") {
    !x
    } else { stop(...) }
    }
    bizarro(c(TRUE, FALSE, FALSE, TRUE, FALSE))
    #> [1] FALSE TRUE TRUE FALSE TRUE
    bizarro(c("abc", "def"))
    #> Error: Don't know how to make bizzaro

    View full-size slide

  31. bizarro <- function(x) {
    if (is.numeric(x)) {
    -x
    } else if (is.logical(x)) {
    !x
    } else { stop(...) }
    }
    bizarro(1:5)
    #> [1] -1 -2 -3 -4 -5
    bizarro(c("abc", "def"))
    #> Error: Don't know how to make bizzaro

    View full-size slide

  32. Tip #4:
    Use proper functions for handling class & type.
    Use simple conditions.
    Extract fussy condition logic into well-named
    function.

    View full-size slide

  33. from googledrive
    is_parental <- function(d) {
    stopifnot(inherits(d, "dribble"))
    kind <- purrr::map_chr(d$drive_resource, "kind")
    mime_type <- purrr::map_chr(d$drive_resource, "mimeType", .default = NA)
    kind == "drive#teamDrive" | mime_type == "application/vnd.google-apps.folder"
    }
    drive_cp <- function(file, ...) {
    ...
    if (is_parental(file)) {
    stop_glue("The Drive API does not copy folders or Team Drives.")
    }
    ...
    }

    View full-size slide

  34. bizarro <- function(x) {
    stopifnot(is.numeric(x) || is.logical(x))
    if (is.numeric(x)) {
    -x
    } else {
    !x
    }
    }
    bizarro(c(TRUE, FALSE, FALSE, TRUE, FALSE))
    #> [1] FALSE TRUE TRUE FALSE TRUE
    bizarro(1:5)
    #> [1] -1 -2 -3 -4 -5
    bizarro(c("abc", "def"))
    #> Error: is.numeric(x) || is.logical(x) is not TRUE

    View full-size slide

  35. Tip #5:
    Hoist quick stop()s and return()s to the top.
    I.e., use a guard clause.
    Clarify and emphasize the happy path.

    View full-size slide

  36. get_some_data <- function(config, outfile) {
    if (config_ok(config)) {
    if (can_write(outfile)) {
    if (can_open_network_connection(config)) {
    data <- parse_something_from_network()
    if(makes_sense(data)) {
    data <- beautify(data)
    write_it(data, outfile)
    return(TRUE)
    } else {
    return(FALSE)
    }
    } else {
    stop("Can't access network")
    }
    } else {
    ## uhm. What was this else for again?
    }
    } else {
    ## maybe, some bad news about ... the config?
    }
    }

    View full-size slide

  37. get_some_data <- function(config, outfile) {
    if (config_bad(config)) {
    stop("Bad config")
    }
    if (!can_write(outfile)) {
    stop("Can't write outfile")
    }
    if (!can_open_network_connection(config)) {
    stop("Can't access network")
    }
    data <- parse_something_from_network()
    if(!makes_sense(data)) {
    return(FALSE)
    }
    data <- beautify(data)
    write_it(data, outfile)
    TRUE
    }

    View full-size slide

  38. get_some_data <- function(config, outfile) {
    if (config_bad(config)) {
    stop("Bad config")
    }
    if (!can_write(outfile)) {
    stop("Can't write outfile")
    }
    if (!can_open_network_connection(config)) {
    stop("Can't access network")
    }
    data <- parse_something_from_network()
    if(!makes_sense(data)) {
    return(FALSE)
    }
    data <- beautify(data)
    write_it(data, outfile)
    TRUE
    }
    get_some_data <- function(config, outfile) {
    if (config_ok(config)) {
    if (can_write(outfile)) {
    if (can_open_network_connection(config)) {
    data <- parse_something_from_network()
    if(makes_sense(data)) {
    data <- beautify(data)
    write_it(data, outfile)
    return(TRUE)
    } else {
    return(FALSE)
    }
    } else {
    stop("Can't access network")
    }
    } else {
    ## uhm. What was this else for again?
    }
    } else {
    ## maybe, some bad news about ... the config?
    }
    }

    View full-size slide

  39. There is no else, there is only if.

    View full-size slide

  40. get_some_data <- function(config, outfile) {
    if (config_bad(config)) {
    stop("Bad config")
    }
    if (!can_write(outfile)) {
    stop("Can't write outfile")
    }
    if (!can_open_network_connection(config)) {
    stop("Can't access network")
    }
    data <- parse_something_from_network()
    if(!makes_sense(data)) {
    return(FALSE)
    }
    data <- beautify(data)
    write_it(data, outfile)
    TRUE
    }
    get_some_data <- function(config, outfile) {
    if (config_ok(config)) {
    if (can_write(outfile)) {
    if (can_open_network_connection(config)) {
    data <- parse_something_from_network()
    if(makes_sense(data)) {
    data <- beautify(data)
    write_it(data, outfile)
    return(TRUE)
    } else {
    return(FALSE)
    }
    } else {
    stop("Can't access network")
    }
    } else {
    ## uhm. What was this else for again?
    }
    } else {
    ## maybe, some bad news about ... the config?
    }
    }

    View full-size slide

  41. TMI
    too much information?

    View full-size slide

  42. TMI
    too much information?
    total meat indentation!

    View full-size slide

  43. get_some_data <- function(config, outfile) {
    if (config_bad(config)) {
    stop("Bad config")
    }
    if (!can_write(outfile)) {
    stop("Can't write outfile")
    }
    if (!can_open_network_connection(config)) {
    stop("Can't access network")
    }
    data <- parse_something_from_network()
    if(!makes_sense(data)) {
    return(FALSE)
    }
    data <- beautify(data)
    write_it(data, outfile)
    TRUE
    }
    get_some_data <- function(config, outfile) {
    if (config_ok(config)) {
    if (can_write(outfile)) {
    if (can_open_network_connection(config)) {
    data <- parse_something_from_network()
    if(makes_sense(data)) {
    data <- beautify(data)
    write_it(data, outfile)
    return(TRUE)
    } else {
    return(FALSE)
    }
    } else {
    stop("Can't access network")
    }
    } else {
    ## uhm. What was this else for again?
    }
    } else {
    ## maybe, some bad news about ... the config?
    }
    }
    →→→

    →→→→
    →→→→
    →→→→
    →→→→

    View full-size slide

  44. get_some_data <- function(config, outfile) {
    if (config_bad(config)) {
    stop("Bad config")
    }
    if (!can_write(outfile)) {
    stop("Can't write outfile")
    }
    if (!can_open_network_connection(config)) {
    stop("Can't access network")
    }
    data <- parse_something_from_network()
    if(!makes_sense(data)) {
    return(FALSE)
    }
    data <- beautify(data)
    write_it(data, outfile)
    TRUE
    }
    get_some_data <- function(config, outfile) {
    if (config_ok(config)) {
    if (can_write(outfile)) {
    if (can_open_network_connection(config)) {
    data <- parse_something_from_network()
    if(makes_sense(data)) {
    data <- beautify(data)
    write_it(data, outfile)
    return(TRUE)
    } else {
    return(FALSE)
    }
    } else {
    stop("Can't access network")
    }
    } else {
    ## uhm. What was this else for again?
    }
    } else {
    ## maybe, some bad news about ... the config?
    }
    }
    →→→

    →→→→
    →→→→
    →→→→
    →→→→
    nested
    conditionals
    Jenny's
    made-up
    metric
    early exits
    17 TMI 1
    3 MMI 0

    View full-size slide

  45. from googledrive
    process_response <- function(res) {
    if (httr::status_code(res) == 204) {
    return(TRUE)
    }
    if (httr::status_code(res) >= 200 && httr::status_code(res) < 300) {
    return(res %>%
    stop_for_content_type() %>%
    httr::content(as = "parsed", type = "application/json"))
    }
    ## 20+ more lines of error handling ...
    }

    View full-size slide

  46. Tip #5c:
    An if block that ends with stop() or
    return() does not require else.
    Recognize your early exits.
    less indentation >> lots of indentation

    View full-size slide

  47. bizarro <- function(x) {
    if (is.numeric(x)) {
    -x
    } else if (is.logical(x)) {
    !x
    } else if (is.character(x)) {
    str_reverse(x)
    } else if (is.factor(x)) {
    levels(x) <- rev(levels(x))
    x
    } else {
    stop(...)
    }
    }

    View full-size slide






  48. bizarro <- function(x) {
    if (is.numeric(x)) {
    -x
    } else if (is.logical(x)) {
    !x
    } else if (is.character(x)) {
    str_reverse(x)
    } else if (is.factor(x)) {
    levels(x) <- rev(levels(x))
    x
    } else {
    stop(...)
    }
    }

    View full-size slide

  49. Tip #6:
    If your conditions deal with class, it's time to get
    object-oriented (OO).
    In CS jargon, use polymorphism.

    View full-size slide

  50. bizarro <- function(x) {
    UseMethod("bizarro")
    }
    bizarro.default <- function(x) {
    stop(
    "Don't know how to make bizzaro <",
    class(x)[[1]], ">",
    call. = FALSE
    )
    }
    S3 OO system

    View full-size slide

  51. bizarro(1:5)
    #> Error: Don't know how to make bizzaro
    bizarro(TRUE)
    #> Error: Don't know how to make bizzaro
    bizarro("abc")
    #> Error: Don't know how to make bizzaro

    View full-size slide

  52. bizarro.numeric <- function(x) -x
    bizarro.logical <- function(x) !x
    bizarro.character <- function(x) str_reverse(x)
    bizarro.factor <- function(x) {
    levels(x) <- rev(levels(x))
    x
    }
    bizarro.data.frame <- function(x) {
    names(x) <- bizarro(names(x))
    x[] <- lapply(x, bizarro)
    x
    }
    S3 OO system

    View full-size slide

  53. str_reverse <- function(x) {
    vapply(
    strsplit(x, ""),
    FUN = function(z) paste(rev(z), collapse = ""),
    FUN.VALUE = ""
    )
    }
    str_reverse(c("abc", "def"))
    #> [1] "cba" "fed"

    View full-size slide

  54. bizarro(1:5)
    #> [1] -1 -2 -3 -4 -5
    bizarro(c(TRUE, FALSE, FALSE, TRUE, FALSE))
    #> [1] FALSE TRUE TRUE FALSE TRUE
    bizarro(c("abc", "def"))
    #> [1] "cba" "fed"
    (m <- factor(month.abb[1:3], levels = month.abb[1:3]))
    #> [1] Jan Feb Mar
    #> Levels: Jan Feb Mar
    bizarro(m)
    #> [1] Mar Feb Jan
    #> Levels: Mar Feb Jan
    bizarro(head(iris, 3))
    #> htgneL.lapeS htdiW.lapeS htgneL.lateP htdiW.lateP seicepS
    #> 1 -5.1 -3.5 -1.4 -0.2 virginica
    #> 2 -4.9 -3.0 -1.4 -0.2 virginica
    #> 3 -4.7 -3.2 -1.3 -0.2 virginica

    View full-size slide

  55. Tip #6 redux:
    Avoid explicit conditionals by creating methods
    that are specific to an object's class.

    View full-size slide

  56. bizarro <- function(x) {
    cls <- class(x)[[1]] ## switching on class is
    switch(
    cls,
    logical = !x,
    integer = ,
    numeric = -x,
    character = str_reverse(x),
    stop(...)
    )
    }

    View full-size slide

  57. from stringr
    str_pad <- function(string,
    width,
    side = c("left", "right", "both"),
    pad = " ") {
    side <- match.arg(side)
    switch(
    side,
    left = stri_pad_left(string, width, pad = pad),
    right = stri_pad_right(string, width, pad = pad),
    both = stri_pad_both(string, width, pad = pad)
    )
    }

    View full-size slide

  58. Tip #7:
    switch() is ideal if you need to dispatch
    different logic, based on a string.
    Tip #7a: You are allowed to write a helper function
    to generate that string.

    View full-size slide

  59. library(tidyverse)
    tibble(
    age_yrs = c(0, 4, 10, 15, 24, 55),
    age_cat = case_when(
    age_yrs < 2 ~ "baby",
    age_yrs < 13 ~ "kid",
    age_yrs < 20 ~ "teen",
    TRUE ~ "adult"
    )
    )
    #> # A tibble: 6 x 2
    #> age_yrs age_cat
    #>
    #> 1 0 baby
    #> 2 4 kid
    #> 3 10 kid
    #> 4 15 teen
    #> 5 24 adult
    #> 6 55 adult

    View full-size slide

  60. ifelse(age_yrs < 2, "baby",
    ifelse(age_yrs < 13, "kid",
    ifelse(age_yrs < 20, "teen",
    "adult"
    )
    )
    )
    alternative to:
    library(tidyverse)
    tibble(
    age_yrs = c(0, 4, 10, 15, 24, 55),
    age_cat = case_when(
    age_yrs < 2 ~ "baby",
    age_yrs < 13 ~ "kid",
    age_yrs < 20 ~ "teen",
    TRUE ~ "adult"
    )
    )
    #> # A tibble: 6 x 2
    #> age_yrs age_cat
    #>
    #> 1 0 baby
    #> 2 4 kid
    #> 3 10 kid
    #> 4 15 teen
    #> 5 24 adult
    #> 6 55 adult

    View full-size slide

  61. Tip #8:
    dplyr::case_when() is ideal if you need to
    dispatch different data, based on data (+ logic).

    View full-size slide

  62. from rlang, purrr
    `%||%` <- function(x, y) {
    if (is_null(x)) y else x
    }

    View full-size slide

  63. from devtools
    github_remote <- function(repo, username = NULL, ...) {
    meta <- parse_git_repo(repo)
    ...
    meta$username <- username %||%
    getOption("github.user") %||%
    stop("Unknown username.")
    ...
    }

    View full-size slide

  64. rstd.io/code-smells
     @jennybc  @JennyBryan
    Write simple conditions.
    Use helper functions.
    Handle class properly.
    Return and exit early.
    polymorphism
    switch()
    case_when()
    %||%

    View full-size slide