4
\$\begingroup\$

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 ## 

##EOF

Here is the Template File that is called:

<?php

/* Marker: F8kR8ikBiF */

/* * * * * * * * * * * * * * * * * * * * * * * * * * * * * * * * * * *
* File Altered By Automated SH script via Cron: overwriteDeferred.sh
* script created by xxxxxxxxxxxxxxxxxxxx 10-24-14 in order to disable
* the built in cron functioanlity of Xxxxxxxx. Cron functionality will
* now be triggered by a System Cron Script: manCronTrigger.sh to run
* once per minute.
* * * * * * * * * * * * * * * * * * * * * * * * * * * * * * * * * * */

{"moreDeferred":false}

/* Oringinal content * START * 


* Oringinal content * END */

// EOF
\$\endgroup\$
1
  • 1
    \$\begingroup\$ I guess all the ### ... ### markers are treated as headlines at the moment. Can you fix the indentation of the bash script, so that it shows up as you actually wrote it? \$\endgroup\$
    – ferada
    Oct 27, 2014 at 18:05

1 Answer 1

3
\$\begingroup\$

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
\$\endgroup\$
0

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge you have read our privacy policy.

Not the answer you're looking for? Browse other questions tagged or ask your own question.