4
\$\begingroup\$

The user selects one of the three categories then makes a note on that selection. Then the note is appended and saved to the appropriate list.

It took me four hours to write this. I'm trying to learn and was wondering if this could be improved or cleaned up at all.

#!/bin/bash

#get the date
date=$(date +%d-%B-%Y)

#save locations
wsave="${HOME}/worknotes.txt"
shsave="${HOME}/shoppingnotes.txt"
scsave="${HOME}/schoolnotes.txt"


#list
while [ true ]
do
read -p "What is this note for?
Work
School
Shopping 
> " topic
case $topic in

    "Work" )
        read -p "
Note
> " wnote
        echo "$date: $wnote" >> "$wsave"
        echo "Note saved to $wsave"
            break
            ;;
    "Shopping" )
        read -p "
Note
> " shnote
        echo "$date: $shnote" >> "$shsave"
        echo "Note saved to $shsave"
            break
            ;;
    "School" )
        read -p "
Note
> " scnote
        echo "$date: $scnote" >> "$scsave"
        echo "Note saved to $scsave"
            break
            ;;
    *) echo "Error: Selection was not on list, try again.
"
            ;;
esac
done
\$\endgroup\$
6
\$\begingroup\$

A couple of points:

  1. You determine the date outside your loop, then loop forever. Every note you make after the first one is going to have the same date stamp.

    A better approach would be to collect the date stamp just as you are writing to the file:

    datestamp=$(...)
    echo "$datestamp: $note" >> ...
    
  2. When shells were originally written, almost everything was a program. The if statement takes an executable as its conditional element. (If you look, you will find a program named [ in /bin so that if [ ... will work.)

    There is a program named true and a program named false. You can run them, and they don't do anything except set their exit codes to appropriate values.

    You don't need to write while [ true ]. Instead, just write while true. This is important because while false will do something different from while [ false ]. You may be surprised ...

  3. As @Vogel612 points out, you have duplicated code. I think you should keep the user feedback close to the user entry, so your checking should happen before typing the note. You can use another variable to hold the destination file path:

    read -p "What is this note for?
    Work
    School
    Shopping 
    > " topic
    destfile=''
    case $topic in
        "Work"     ) destfile="$wsave" ;;
        "School"   ) destfile="$scsave" ;;
        "Shopping" ) destfile="$shsave" ;;
        * ) echo "Error: Selection was not on list, try again.\n" ;;
    esac
    
    if [ "$destfile" ]; then 
        read -p "\nNote\n> " note
        echo "$date: $note" >> "$destfile"
        echo "Note saved to $destfile"
    fi
    
\$\endgroup\$
  • \$\begingroup\$ Awesome, thanks! I added a quit option using a break command under shopping. I added a -e to the echo commands becuase the newlines weren't coming through. Read that printf is a subsitute, but that didn't display the way I wanted to. Moved the date variable to right before the ehco statement that calls it. \$\endgroup\$ – Ryan R Mar 13 at 18:35
4
\$\begingroup\$

Extract common code into a common block. You only need the case to determine the save-location. The note itself can be read before switching:

#...
read -p "What is this note for?
Work
School
Shopping
> " topic
read -p "
Note 
> "  note
save=""
case $topic in
  "Work")
    save=$wsave
    break
    ;;
  "School")
    save=scsave
    break
    ;;
  "Shopping")
    save=$shsave
    break
    ;;
  *) echo "Error: Selection was not on list, try again.
"
    ;;
esac
if [[ $save!="" ]]; then
  echo "$date: $note" >> "$save"
  echo "Note saved to $save"
fi
#...

this removes the duplication in the case-blocks and still allows you to clearly work with what you expect for every note.


The variable names could use a bit more... characters in general. You could even use snake_case to differentiate between words and all that. This allows you to make the code significantly more speaking.


The comment #list is really not adding any value. #get the date is already clearly outlined by the code, maybe a more human readable format explanation might be useful there. #save locations should be completely replaceable with proper variable names.


A word on the save locations you are using. As it stands, these are totally visible and will clutter the user's home directory. You should consider making these hidden by default by prefixing the filenames with a ., maybe even put them into a separate folder in the home-directory.

\$\endgroup\$
  • \$\begingroup\$ Thanks for the ideas! I would like it though if the note only saved after the user selected an option from the predefined list. With your code if the user misspelled a topic name there potentially long note wouldn't be saved, right? I agree the comments I make aren't very helpful. I made a separate directory for the notes. \$\endgroup\$ – Ryan R Mar 13 at 17:32

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.