Take the 2-minute tour ×
Code Review Stack Exchange is a question and answer site for peer programmer code reviews. It's 100% free, no registration required.

I'm new to Code Review and new to writing bash scripts as well. I was wondering if I could get someone's advice on a part of the script I'm working on.

Intended purpose of code

Check if a package exists using dpkg, if it doesn't, offer to install it for the user. This snippet is part of a larger script that installs a particular Conky configuration along with all of its dependencies with minimal effort from the user.

Concerns

  • I feel as though there is a more elegant way to check if a package is installed using dpkg (code was found on stackoverflow).
  • Is there a better way of handling the (y/n) response?

Here is the code that I am using:

declare -a packages=("conky-all" "lm-sensors");

for i in "${packages[@]}"; do
    if [ $(dpkg-query -W -f='${Status}' $i 2>/dev/null | grep -c "ok installed") -eq 0 ]; then
        echo "$i is not installed, would you like to install it now? (Y/N)";
        read response
        if [ "$response" == "y" ] || [ "$response" == "Y" ]; then
            sudo apt-get install "$i";
        else
            echo "Skipping the installation of $i...";
            echo "Please note that this Conky configuration will not work without the $i package.";
        fi
    else
        echo "The $i package has already been installed.";
    fi
done
share|improve this question

1 Answer 1

up vote 2 down vote accepted

Instead of this:

    if [ $(dpkg-query -W -f='${Status}' $i 2>/dev/null | grep -c "ok installed") -eq 0 ]; then

A better way to write the same thing:

    if ! dpkg-query -W -f='${Status}' $i 2>/dev/null | grep -q "ok installed"; then

Instead of this:

        if [ "$response" == "y" ] || [ "$response" == "Y" ]; then

A simpler way to write is:

        if [[ $response == [yY]* ]]; then

This is not example the same. It will match anything that starts with "y" or "Y". If you want to match strictly only those letters, just drop the * from the pattern:

        if [[ $response == [yY] ]]; then

Finally, all the ; at line endings are unnecessary. The purpose of ; is to separate multiple statements on the same line. Line breaks naturally serve as statement separators.

share|improve this answer
    
Thank you very much. Just for the sake of clarity, why are the double brackets necessary for the "if [[ $response == [yY] ]]" statement? –  John P. Apr 29 at 20:05
1  
[ ... ] cannot match patterns like that. [[ ... ]] is more modern and sophisticated, it can do this. –  janos Apr 29 at 20:07
1  
Just make it a habit to quote all variable expansions. It's not worth it to save a few quote characters. –  200_success Apr 30 at 0:55
    
@200_success I guess you're right. I dropped that point about quoting –  janos Apr 30 at 4:48

Your Answer

 
discard

By posting your answer, you agree to the privacy policy and terms of service.

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