Append a note to one of three files based on user choice
$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
beginner bash linux shell
New contributor
$endgroup$
add a comment |
$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
beginner bash linux shell
New contributor
$endgroup$
add a comment |
$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
beginner bash linux shell
New contributor
$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
beginner bash linux shell
New contributor
New contributor
edited 6 hours ago
200_success
130k17153419
130k17153419
New contributor
asked 6 hours ago
Ryan RRyan R
162
162
New contributor
New contributor
add a comment |
add a comment |
2 Answers
2
active
oldest
votes
$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$
add a comment |
$begingroup$
A couple of points:
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" >> ...
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 thatif [ ...
will work.)
There is a program named
true
and a program namedfalse
. 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 writewhile true
. This is important becausewhile false
will do something different fromwhile [ false ]
. You may be surprised ...
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
$endgroup$
add a comment |
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.
Sign up or log in
StackExchange.ready(function () {
StackExchange.helpers.onClickDraftSave('#login-link');
});
Sign up using Google
Sign up using Facebook
Sign up using Email and Password
Post as a guest
Required, but never shown
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
$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$
add a comment |
$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$
add a comment |
$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$
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.
answered 5 hours ago
Vogel612♦Vogel612
21.8k447130
21.8k447130
add a comment |
add a comment |
$begingroup$
A couple of points:
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" >> ...
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 thatif [ ...
will work.)
There is a program named
true
and a program namedfalse
. 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 writewhile true
. This is important becausewhile false
will do something different fromwhile [ false ]
. You may be surprised ...
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
$endgroup$
add a comment |
$begingroup$
A couple of points:
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" >> ...
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 thatif [ ...
will work.)
There is a program named
true
and a program namedfalse
. 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 writewhile true
. This is important becausewhile false
will do something different fromwhile [ false ]
. You may be surprised ...
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
$endgroup$
add a comment |
$begingroup$
A couple of points:
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" >> ...
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 thatif [ ...
will work.)
There is a program named
true
and a program namedfalse
. 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 writewhile true
. This is important becausewhile false
will do something different fromwhile [ false ]
. You may be surprised ...
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
$endgroup$
A couple of points:
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" >> ...
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 thatif [ ...
will work.)
There is a program named
true
and a program namedfalse
. 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 writewhile true
. This is important becausewhile false
will do something different fromwhile [ false ]
. You may be surprised ...
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
answered 4 hours ago
Austin HastingsAustin Hastings
7,0821233
7,0821233
add a comment |
add a comment |
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.
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.
Sign up or log in
StackExchange.ready(function () {
StackExchange.helpers.onClickDraftSave('#login-link');
});
Sign up using Google
Sign up using Facebook
Sign up using Email and Password
Post as a guest
Required, but never shown
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
Sign up or log in
StackExchange.ready(function () {
StackExchange.helpers.onClickDraftSave('#login-link');
});
Sign up using Google
Sign up using Facebook
Sign up using Email and Password
Post as a guest
Required, but never shown
Sign up or log in
StackExchange.ready(function () {
StackExchange.helpers.onClickDraftSave('#login-link');
});
Sign up using Google
Sign up using Facebook
Sign up using Email and Password
Post as a guest
Required, but never shown
Sign up or log in
StackExchange.ready(function () {
StackExchange.helpers.onClickDraftSave('#login-link');
});
Sign up using Google
Sign up using Facebook
Sign up using Email and Password
Sign up using Google
Sign up using Facebook
Sign up using Email and Password
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