Append a note to one of three files based on user choice












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









share|improve this question









New contributor




Ryan R is a new contributor to this site. Take care in asking for clarification, commenting, and answering.
Check out our Code of Conduct.







$endgroup$

















    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









    share|improve this question









    New contributor




    Ryan R is a new contributor to this site. Take care in asking for clarification, commenting, and answering.
    Check out our Code of Conduct.







    $endgroup$















      4












      4








      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









      share|improve this question









      New contributor




      Ryan R is a new contributor to this site. Take care in asking for clarification, commenting, and answering.
      Check out our Code of Conduct.







      $endgroup$




      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






      beginner bash linux shell






      share|improve this question









      New contributor




      Ryan R is a new contributor to this site. Take care in asking for clarification, commenting, and answering.
      Check out our Code of Conduct.











      share|improve this question









      New contributor




      Ryan R is a new contributor to this site. Take care in asking for clarification, commenting, and answering.
      Check out our Code of Conduct.









      share|improve this question




      share|improve this question








      edited 9 hours ago









      200_success

      130k17153419




      130k17153419






      New contributor




      Ryan R is a new contributor to this site. Take care in asking for clarification, commenting, and answering.
      Check out our Code of Conduct.









      asked 9 hours ago









      Ryan RRyan R

      212




      212




      New contributor




      Ryan R is a new contributor to this site. Take care in asking for clarification, commenting, and answering.
      Check out our Code of Conduct.





      New contributor





      Ryan R is a new contributor to this site. Take care in asking for clarification, commenting, and answering.
      Check out our Code of Conduct.






      Ryan R is a new contributor to this site. Take care in asking for clarification, commenting, and answering.
      Check out our Code of Conduct.






















          2 Answers
          2






          active

          oldest

          votes


















          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.






          share|improve this answer









          $endgroup$





















            4












            $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 "nNoten> " note
              echo "$date: $note" >> "$destfile"
              echo "Note saved to $destfile"
              fi







            share|improve this answer









            $endgroup$













              Your Answer





              StackExchange.ifUsing("editor", function () {
              return StackExchange.using("mathjaxEditing", function () {
              StackExchange.MarkdownEditor.creationCallbacks.add(function (editor, postfix) {
              StackExchange.mathjaxEditing.prepareWmdForMathJax(editor, postfix, [["\$", "\$"]]);
              });
              });
              }, "mathjax-editing");

              StackExchange.ifUsing("editor", function () {
              StackExchange.using("externalEditor", function () {
              StackExchange.using("snippets", function () {
              StackExchange.snippets.init();
              });
              });
              }, "code-snippets");

              StackExchange.ready(function() {
              var channelOptions = {
              tags: "".split(" "),
              id: "196"
              };
              initTagRenderer("".split(" "), "".split(" "), channelOptions);

              StackExchange.using("externalEditor", function() {
              // Have to fire editor after snippets, if snippets enabled
              if (StackExchange.settings.snippets.snippetsEnabled) {
              StackExchange.using("snippets", function() {
              createEditor();
              });
              }
              else {
              createEditor();
              }
              });

              function createEditor() {
              StackExchange.prepareEditor({
              heartbeatType: 'answer',
              autoActivateHeartbeat: false,
              convertImagesToLinks: false,
              noModals: true,
              showLowRepImageUploadWarning: true,
              reputationToPostImages: null,
              bindNavPrevention: true,
              postfix: "",
              imageUploader: {
              brandingHtml: "Powered by u003ca class="icon-imgur-white" href="https://imgur.com/"u003eu003c/au003e",
              contentPolicyHtml: "User contributions licensed under u003ca href="https://creativecommons.org/licenses/by-sa/3.0/"u003ecc by-sa 3.0 with attribution requiredu003c/au003e u003ca href="https://stackoverflow.com/legal/content-policy"u003e(content policy)u003c/au003e",
              allowUrls: true
              },
              onDemand: true,
              discardSelector: ".discard-answer"
              ,immediatelyShowMarkdownHelp:true
              });


              }
              });






              Ryan R is a new contributor. Be nice, and check out our Code of Conduct.










              draft saved

              draft discarded


















              StackExchange.ready(
              function () {
              StackExchange.openid.initPostLogin('.new-post-login', 'https%3a%2f%2fcodereview.stackexchange.com%2fquestions%2f215297%2fappend-a-note-to-one-of-three-files-based-on-user-choice%23new-answer', 'question_page');
              }
              );

              Post as a guest















              Required, but never shown

























              2 Answers
              2






              active

              oldest

              votes








              2 Answers
              2






              active

              oldest

              votes









              active

              oldest

              votes






              active

              oldest

              votes









              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.






              share|improve this answer









              $endgroup$


















                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.






                share|improve this answer









                $endgroup$
















                  4












                  4








                  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.






                  share|improve this answer









                  $endgroup$



                  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.







                  share|improve this answer












                  share|improve this answer



                  share|improve this answer










                  answered 8 hours ago









                  Vogel612Vogel612

                  21.8k447130




                  21.8k447130

























                      4












                      $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 "nNoten> " note
                        echo "$date: $note" >> "$destfile"
                        echo "Note saved to $destfile"
                        fi







                      share|improve this answer









                      $endgroup$


















                        4












                        $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 "nNoten> " note
                          echo "$date: $note" >> "$destfile"
                          echo "Note saved to $destfile"
                          fi







                        share|improve this answer









                        $endgroup$
















                          4












                          4








                          4





                          $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 "nNoten> " note
                            echo "$date: $note" >> "$destfile"
                            echo "Note saved to $destfile"
                            fi







                          share|improve this answer









                          $endgroup$



                          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 "nNoten> " note
                            echo "$date: $note" >> "$destfile"
                            echo "Note saved to $destfile"
                            fi








                          share|improve this answer












                          share|improve this answer



                          share|improve this answer










                          answered 7 hours ago









                          Austin HastingsAustin Hastings

                          7,1121233




                          7,1121233






















                              Ryan R is a new contributor. Be nice, and check out our Code of Conduct.










                              draft saved

                              draft discarded


















                              Ryan R is a new contributor. Be nice, and check out our Code of Conduct.













                              Ryan R is a new contributor. Be nice, and check out our Code of Conduct.












                              Ryan R is a new contributor. Be nice, and check out our Code of Conduct.
















                              Thanks for contributing an answer to Code Review Stack Exchange!


                              • Please be sure to answer the question. Provide details and share your research!

                              But avoid



                              • Asking for help, clarification, or responding to other answers.

                              • Making statements based on opinion; back them up with references or personal experience.


                              Use MathJax to format equations. MathJax reference.


                              To learn more, see our tips on writing great answers.




                              draft saved


                              draft discarded














                              StackExchange.ready(
                              function () {
                              StackExchange.openid.initPostLogin('.new-post-login', 'https%3a%2f%2fcodereview.stackexchange.com%2fquestions%2f215297%2fappend-a-note-to-one-of-three-files-based-on-user-choice%23new-answer', 'question_page');
                              }
                              );

                              Post as a guest















                              Required, but never shown





















































                              Required, but never shown














                              Required, but never shown












                              Required, but never shown







                              Required, but never shown

































                              Required, but never shown














                              Required, but never shown












                              Required, but never shown







                              Required, but never shown







                              Popular posts from this blog

                              Accessing regular linux commands in Huawei's Dopra Linux

                              Can't connect RFCOMM socket: Host is down

                              Kernel panic - not syncing: Fatal Exception in Interrupt