8
\$\begingroup\$

Background

I've provided two simple functions that I use to print a full path to the last modified file within a directory. I don't code in bash frequently so I'm looking for a generic feedback on what to do / avoid when writing small utility functions in bash.

Code

Last file

The function prints a full path to last modified file in a specific or current directory (if no path is provided).

function last_file () {
    if [[ -n "$1" ]]
    then
        (cd "$1" && echo "$(pwd -P)/$(ls -rt | tail -n 1)")
    else 
        echo "$(pwd -P)/$(ls -rt | tail -n 1)"
    fi
}

Last download

As above but prints last modified file in the ~/Downloads.

function last_download () {
    (cd ~/Downloads && last_file)

}
\$\endgroup\$
4
  • 1
    \$\begingroup\$ There isn't much code to look at but hey, small questions can gain small answers I suppose :) \$\endgroup\$ Commented Dec 18, 2022 at 19:08
  • \$\begingroup\$ @MaartenBodewes That's true, I was thinking that I may be relaying on subshells too frivolously so I was wondering if someone is going to comment on that... \$\endgroup\$
    – Konrad
    Commented Dec 18, 2022 at 19:19
  • \$\begingroup\$ Hmm, OK, maybe a good idea to indicate why you chose that instead of one of the other options like braces in that case. \$\endgroup\$ Commented Dec 18, 2022 at 19:41
  • \$\begingroup\$ last_download() { last_file ~/Downloads; } \$\endgroup\$ Commented Dec 19, 2022 at 13:13

3 Answers 3

6
\$\begingroup\$

I would like to remove the duplicated command.

last_file () {
    (
        set -e
        [[ -n "$1" ]] && cd "$1"
        printf '%s/%s\n' "$(pwd -P)" "$(ls -rt | tail -n 1)"
    )
}
  • The whole body is run in a subshell.
  • set -e makes the subshell abort if the given argument is not a directory. You did something similar with cd "$1" && echo ...
  • printf is better than echo
\$\endgroup\$
0
6
\$\begingroup\$

Test argument count

Testing [ -n "$1" ] is a failure if we're running with set -u (and I do recommend set -u in general). You can test "${1-}" instead to safely get an empty string when given no arguments.

But really, we should return an error if we have too many arguments, so I suggest case $# in instead.

Prefer head to tail

In a pipe, using head can be more efficient than tail, since the writing side can stop (with SIGPIPE) when head has enough lines and exits. In this case, removing -r from the ls command allows us

Don't process the output of ls

ls will print file names separated by newlines, and those newlines are indistinguishable from any newline that might be within a file name.

The other idiosyncrasy of ls is that it will omit all names that begin with ., which might be what you want, but that wasn't mentioned.

function keyword is unnecessary

Most shell scripters omit the function keyword, for a more portable function definition.


Alternative implementation

This is something I've looked at, and have now posted for its own review.

I created a function that takes one or more arguments and returns the newest file of those, using test to compare modification times (thus avoiding any problems with embedded newlines):

last_mtime() {
    test -e "${1-}" || return 1
    last=$1; shift
    for i
    do
        test -e "$i" || return 1
        test "$last" -nt "$i" || last=$i
    done
    printf '%s\n' "$last"
}

Then the downloads function looks like

last_download() {
    # N.B. ignores anything starting with '.', unless 'dotglob'
    # is set in Bash options
    last_mtime ~/Downloads/*
}
\$\endgroup\$
0
3
\$\begingroup\$

Using subshells frivolously

In a comment you expressed concern about using subshells frivolously. Often there's a tradeoff between using a subshell or writing more complex code to avoid. There's an overhead of subshells so it's a good practice to avoid them, when the alternative is simple enough.

An important factor is if the script will be used in a loop or not. One-off uses of subshell won't be noticeable, but that can change when called in a nested loop.

For getting the absolute path of another directory, I think it's a fine tradeoff to use a subshell, because it's very simple to write, and the alternative is complex and error prone, that is, computing the same in native Bash, especially the processing of symbolic links.

I think you did right to separate the cases of the current directory and a different directory, and not using a subshell in the first one, since it's unnecessary.

Aside from the (cd ...), there are other subshells in this code that can be avoided, see the next sections.

Consider $PWD instead of pwd -P

If you don't have a very specific need for the effect of pwd -P, consider using simply $PWD. It's not exactly the same thing with respect to symbolic links, but it does give you an absolute path that might be good enough for your intents and purposes.

Consider cmd ... instead of echo "$(cmd ...)"

Instead of:

echo "$(pwd -P)/$(ls -rt | tail -n 1)"

You could eliminate one subshell and the echo with the equivalent:

ls -t "$(pwd -P)"/* | head -n 1

Avoiding duplication

As another answer pointed out, there is duplicate logic in the implementation of the cases of the current directory and another directory. A simple way to avoid the duplication is a recursive call to the same function, that is (cd "$1" && last_file). I prefer this over the solution offered in that answer, because this avoids an unnecessary subshell in the case of the current directory.

Error handling

The script doesn't handle errors. When the specified directory doesn't exist, it would be better to return a failure explicitly, otherwise the output might be misleading.

Alternative implementation

Putting the above suggestions together, also assuming set -u is in effect, as another answer suggested:

last_file() {
    if [[ "${1+x}" ]]
    then
        [ -d "$1" ] || return 1
        (cd "$1" && last_file)
    else 
        ls -t "$PWD"/* | head -n 1
    fi
}
\$\endgroup\$

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge you have read our privacy policy.

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