2
\$\begingroup\$

I'm in the process of understanding shell scripting - this is my simple code for adding test files for python unittest

I'm looking for best practice pointers, variable naming, and of course comments on structure and readability.

EDIT: Script is called by, for example

./create_tests.sh math tensor_add
dir_name="$1"
file_name="$2"
test_dir="test/${dir_name}"
file_in_dir="${test_dir}/${file_name}.py"

addFile() {
    if [[ -f "${file_in_dir}" ]]
    then
        echo "File already exists in directory, exiting."
        exit 0
    else
        touch "${file_in_dir}"
    fi
}

if [[ -d "${test_dir}" ]]
then
    echo "Directory already exists, adding the file to this directory."
    addFile
else
    echo "Directory and __init__.py created."
    mkdir "${test_dir}"
    touch "${test_dir}/__init__.py"
    addFile
fi
\$\endgroup\$
2
\$\begingroup\$
  1. Add a shebang.
  2. Keep naming consistent, I personally prefer and follow Google's style guide; you may use a different guide if available.
  3. Try to make your script POSIX compliant, for a wider target reach (no need to rely on [[ expression from bash)
  4. Define a function to create the test directory.
  5. Use shellcheck.
  6. The echo "Directory and __init__.py created." should happen after the creation has happened.
  7. Use mkdir -p to allow for nested directories to also be created (if needed).
  8. Let script throw error and stop as early as possible on errors. This is achieved with set -e.

Together:

#!/bin/sh

set -e

dir_name="$1"
file_name="$2"
test_dir="test/${dir_name}"
file_in_dir="${test_dir}/${file_name}.py"

add_file() {
    if test -f "${file_in_dir}"
    then
        echo "File already exists in directory, exiting."
        exit 0
    else
        touch "${file_in_dir}"
    fi
}

create_dir() {
    mkdir -p "${test_dir}"
    touch "${test_dir}/__init__.py"
    echo "Directory and __init__.py created."
}

[ ! -d "${test_dir}" ] && create_dir "${test_dir}"

add_file

The chunk of variable declarations (dir_name, file_name etc.) could also be localised to functions.

\$\endgroup\$
3
  • 1
    \$\begingroup\$ You missed "extract common lines from if/else" from the list - though you've demonstrated in your reworked code. \$\endgroup\$ – Toby Speight Feb 3 at 13:03
  • \$\begingroup\$ Thank you for your brilliant advice. One question: what is the difference between the double brackets and single brackets in my contra your implementation? Does it have to do with POSIX compliance? \$\endgroup\$ – Edwin Carlsson Feb 3 at 13:35
  • \$\begingroup\$ stackoverflow.com/q/669452/2002471 explains why double brackets are a good thing. Striving for POSIX-compliance after bash and Linux has taken over the world is an interesting prioritization. I'd hope that code readability and maintainability are more important these days. Going from if [[ to if test makes sense to people who understand how shell if's work, but I'd say it makes it less readable and intuitive for most folks. \$\endgroup\$ – chicks Feb 3 at 14: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.