HiveBrain v1.2.0
Get Started
← Back to all entries
patternbashMinor

Bash Script - File Comment out & Notate

Submitted by: @import:stackexchange-codereview··
0
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 ##

Solution

No need to use quotes here, though it's not a problem:

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"
fi


When 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
fi

Code 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.