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\$

1 Answer 1

Reset to default
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\$ Commented Feb 3, 2021 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\$ Commented Feb 3, 2021 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\$ Commented Feb 3, 2021 at 14:11

You must log in to answer this question.

Start asking to get answers

Find the answer to your question by asking.

Ask question

Explore related questions

See similar questions with these tags.