patternbashMinor
Bash Script - File Comment out & Notate
Viewed 0 times
scriptcommentfilenotateoutbash
Problem
I have been writing my own Bash Scripts for a little while now. I am getting a better comprehension of it and I would like to start refining my code to be more elegant. I have recently developed this code to take a PHP file and comment out the code and add new code in.
If you could let me know how I could do better I would greatly appreciate it.
Bash Script:
```
## Set included files ## START ##
source /scripts/_inc/siteList.sh ## Description: Loads $siteList Array
## Set included files ## END ##
## Set Variables ## START ##
## Files in Use ## START ##
useFile='deferred.php'
tempFile='/scripts/_inc/deferredTextTemplate.php'
## Files in Use ## END ##
## File Contents ## START ##
newTop=$(head -n 15 $tempFile)
newBottom=$(tail -n 4 $tempFile)
## File Contents ## END ##
## Set Variables ## END ##
## Set Functions ## START ##
changeFile () {
## Info About Action to be intered into File.
workInfo="\n// Edited: $(date)\n// Directory: $2"
logTag="$(date): [$3 @ $2]"
if [ -f "$1" ] ## Description: Does the file exist?
then
if grep -q F8kR8ikBiF "$1"; ## Description: Does the file contain this marker?
then
printf "%s Marker exists no changes necessary.\n" "$logTag" ## Description: Marker Exists!
else
## Description: Print to Files that exist without Marker.
orgFileCont=$(tail -n +2 $1 | sed -r 's/\/\+ (.) \*+\//\/\/ \1/')
printf "%s\n%s\n%s\n%s\n" "$newTop" "$workInfo" "$orgFileCont" "$newBottom" > $1
printf "%s File edited.\n" "$logTag"
fi
else
printf "%s File not found.\n" "$logTag" ## Description: File does not exist.
fi
}
## Set Functions ## END ##
## Set actions ## START ##
## Loop though all direcotries listed in $siteList array and perform action
for siteDirectory in "${siteList[@]}"
do
fileLocation="/site/$siteDirectory/$useFile"
changeFile "$fileLocation" "$siteDirectory" "$useFile"
done
## Set actions ## END ##
If you could let me know how I could do better I would greatly appreciate it.
Bash Script:
```
## Set included files ## START ##
source /scripts/_inc/siteList.sh ## Description: Loads $siteList Array
## Set included files ## END ##
## Set Variables ## START ##
## Files in Use ## START ##
useFile='deferred.php'
tempFile='/scripts/_inc/deferredTextTemplate.php'
## Files in Use ## END ##
## File Contents ## START ##
newTop=$(head -n 15 $tempFile)
newBottom=$(tail -n 4 $tempFile)
## File Contents ## END ##
## Set Variables ## END ##
## Set Functions ## START ##
changeFile () {
## Info About Action to be intered into File.
workInfo="\n// Edited: $(date)\n// Directory: $2"
logTag="$(date): [$3 @ $2]"
if [ -f "$1" ] ## Description: Does the file exist?
then
if grep -q F8kR8ikBiF "$1"; ## Description: Does the file contain this marker?
then
printf "%s Marker exists no changes necessary.\n" "$logTag" ## Description: Marker Exists!
else
## Description: Print to Files that exist without Marker.
orgFileCont=$(tail -n +2 $1 | sed -r 's/\/\+ (.) \*+\//\/\/ \1/')
printf "%s\n%s\n%s\n%s\n" "$newTop" "$workInfo" "$orgFileCont" "$newBottom" > $1
printf "%s File edited.\n" "$logTag"
fi
else
printf "%s File not found.\n" "$logTag" ## Description: File does not exist.
fi
}
## Set Functions ## END ##
## Set actions ## START ##
## Loop though all direcotries listed in $siteList array and perform action
for siteDirectory in "${siteList[@]}"
do
fileLocation="/site/$siteDirectory/$useFile"
changeFile "$fileLocation" "$siteDirectory" "$useFile"
done
## Set actions ## END ##
Solution
No need to use quotes here, though it's not a problem:
On the other hand, it might be a good idea to double-quote
It's hard to guess the meaning of the parameters
One way to improve that is to document them in a comment.
An even better way would be to reassign them to local variables with descriptive names.
The marker
It would be better to put it in a variable with a good name,
defined near the top of the script.
You were careful to double-quote most variables correctly, but not always:
You should double-quote the
Also, the
When you have an if-else block like this:
When there are too many lines in the middle,
it can be more readable to invert the if-else branches:
useFile='deferred.php'
tempFile='/scripts/_inc/deferredTextTemplate.php'On the other hand, it might be a good idea to double-quote
$tempFile here:newTop=$(head -n 15 $tempFile)
newBottom=$(tail -n 4 $tempFile)It's hard to guess the meaning of the parameters
$2 and $3 here:changeFile () {
## Info About Action to be intered into File.
workInfo="\n// Edited: $(date)\n// Directory: $2"
logTag="$(date): [$3 @ $2]"One way to improve that is to document them in a comment.
An even better way would be to reassign them to local variables with descriptive names.
The marker
F8kR8ikBiF is buried deep within the script:if grep -q F8kR8ikBiF "$1"; ## Description: Does the file contain this marker?It would be better to put it in a variable with a good name,
defined near the top of the script.
You were careful to double-quote most variables correctly, but not always:
orgFileCont=$(tail -n +2 $1 | sed -r 's/\/\*+ (.*) \*+\//\/\/ \1/')You should double-quote the
$1 there.Also, the
\/ make the expression hard to read. It would be better to use a different separator, for example ?:... | sed -r 's?/\*+ (.*) \*+/?// \1?'When you have an if-else block like this:
if [ -f "$1" ]
then
# ... many many lines
# ... many many lines
# ... many many lines
else
printf "%s File not found.\n" "$logTag"
fiWhen there are too many lines in the middle,
it can be more readable to invert the if-else branches:
if [ ! -f "$1" ]
then
printf "%s File not found.\n" "$logTag"
else
# ... many many lines
# ... many many lines
# ... many many lines
fiCode Snippets
useFile='deferred.php'
tempFile='/scripts/_inc/deferredTextTemplate.php'newTop=$(head -n 15 $tempFile)
newBottom=$(tail -n 4 $tempFile)changeFile () {
## Info About Action to be intered into File.
workInfo="\n// Edited: $(date)\n// Directory: $2"
logTag="$(date): [$3 @ $2]"if grep -q F8kR8ikBiF "$1"; ## Description: Does the file contain this marker?orgFileCont=$(tail -n +2 $1 | sed -r 's/\/\*+ (.*) \*+\//\/\/ \1/')Context
StackExchange Code Review Q#68099, answer score: 3
Revisions (0)
No revisions yet.