6

I have this shellcheck warning I can't figure out:

In /mnt/e/bin/iconic line 540:
            printf "FALSE|" >> "$IconsRaw"           # Select field number 1
            ^-- SC2129: Consider using { cmd1; cmd2; } >> file instead of individual redirects.

I've noticed many of us here use shellcheck to fix our bash scripts / shell commands so I hope the question is on topic.


As per comments posting relevant section of bash script:

    if [[ "$X" == "?" || "$Y" == "?" ]] ; then
        : # Bad X or Y offset usually "Link to Name.ext~" (backup name)
    else
        let i++
        printf "FALSE|" >> "$IconsRaw"           # Select field number 1
        printf "%s|" "$i" >> "$IconsRaw"         # 2
        printf "%s|" "${File##*/}" >> "$IconsRaw"
        printf "%s|" "$Linkless" >> "$IconsRaw"  # 4
        printf "%s|" "$Date" >> "$IconsRaw"      # 5
        printf "%s|" "$X" >> "$IconsRaw"         # 6
        echo   "$Y" >> "$IconsRaw"               # 7
    fi

Solution

Thanks to accepted answer and comments I've learned that shellcheck not only catches errors in your code, but also suggests performance improvements. In this case the filename $IconsRaw was being opened and closed many times with each printf and echo.

The more efficient bash code:

    # X,Y screen coordinates invalid on backup files ending with "~"
    ! [[ "$X" == "?" || "$Y" == "?" ]] && { let i++; echo \
        "FALSE|$i|${File##*/}|$Linkless|$Date|$X|$Y" >> "$IconsRaw"; }
3
  • 1
    I think it's just suggesting doing a single write instead of multiple open/seek/write operations - but it might be clearer if you posted a larger sample of the script Jun 1, 2019 at 1:46
  • 2
    When the message is too terse, you can look at the wiki page for SC2129 which has a full length description with examples Jun 1, 2019 at 8:09
  • @thatotherguy Your comment could be an answer if it included relevant parts of the link in the body. It would help others and I for one would gladly upvote it. Jun 1, 2019 at 15:07

1 Answer 1

8

I assume your script has multiple instances of >> "$IconsRaw". That message is suggesting redirecting the output only once and grouping the commands in a subshell. Presumably to avoid the overhead of opening and closing the file multiple times.

So, instead of this:

    printf "FALSE|" >> "$IconsRaw"           # Select field number 1
    printf "%s|" "$i" >> "$IconsRaw"         # 2
    printf "%s|" "${File##*/}" >> "$IconsRaw"
    printf "%s|" "$Linkless" >> "$IconsRaw"  # 4
    printf "%s|" "$Date" >> "$IconsRaw"      # 5
    printf "%s|" "$X" >> "$IconsRaw"         # 6
    echo   "$Y" >> "$IconsRaw"               # 7

This:

{
    printf "FALSE|"            # Select field number 1
    printf "%s|" "$i"          # 2
    printf "%s|" "${File##*/}" 
    printf "%s|" "$Linkless"   # 4
    printf "%s|" "$Date"       # 5
    printf "%s|" "$X"          # 6
    printf "%s\n" "$Y"         # 7
} >> "$IconsRaw"

Bu that's also a needless repetition of printf and it is more efficient to just do:

printf '%s|%s|%s|%s|%s|%s|%s\n' \
      'FALSE' "$i" "${File##*/}" "$Linkless" \
      "$Date" "$X" "$Y" >> "$IconsRaw"
3
  • 1
    printf also can write all arguments using only one format specifier provided. So you could have one printf and multiple args to it as alternative. Or use a function. Functions support redirection in declaration Jun 1, 2019 at 3:11
  • @WinEunuuchs2Unix please see updated answer. There's no need for multiple calls to printf at all and if you only want \n on the last field, then just only use \n for the last field.
    – terdon
    Jun 1, 2019 at 12:11
  • @SergiyKolodyazhnyy excellent point of concatenating all fields into single printf call. By the same token a single echo could be used with shorter code (no %s) but I wonder if that is less efficient? Terdon thanks for your answer. Accepting now. Jun 1, 2019 at 15:11

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.