3
\$\begingroup\$

I am using sed command to comment in or out sections in my model according to user input (commenting is done by prefixing the line with !). Once this is done, I rebuild my model to get a new executable. An example of a model file (named as scm_code_ex.txt in the script) is:

code
code
code-auto
code-auto
code-auto
code
code
code
code
code-agg
code-agg
code-agg
code
code
code
code
code
code
code
code

My script asks the user which sections of the code to comment out (off means comment out):

#!/bin/bash
# This script will comment in or out sections in the code text file
echo "- Aggregation (on) or (off) (type in ans)?"
read agg

echo "- Autoconversion (on) or (off) (type in ans)?"
read auto


#Revert to original with everything ON.

#On autoconversion
    sed -e'3s/\!//' scm_code_ex.txt >scm_code_ex.tmp
    sed -e'4s/\!//' scm_code_ex.tmp >scm_code_ex.tmp2
    sed -e'5s/\!//' scm_code_ex.tmp2 >scm_code_ex.tmp3

#On aggreagation
    sed -e '10s/\!//' scm_code_ex.tmp3 > scm_code_ex.tmp4
    sed -e '11s/\!//' scm_code_ex.tmp4 > scm_code_ex.tmp5
    sed -e '12s/\!//' scm_code_ex.tmp5 > scm_code_ex.txt


#Remove unwanted files
rm *.tmp*


#Modifications according to conditions

if [ "$agg" == 'on' ] && [ "$auto" == 'on' ]; then
#the conditional statemet above needs spacing in 
#all the right steps otherwise it will not work

echo ""
echo "agg on ... auto on "
echo "Rebuild model now"
echo ""

elif [ "$agg" == 'off' ] && [ "$auto" == 'off' ]; then

#commenting out autoconversion section lines 3-5
    sed -e "3s/^/!/g" scm_code_ex.txt > scm_code_ex.tmp
    sed -e "4s/^/!/" scm_code_ex.tmp > scm_code_ex.tmp2
    sed -e "5s/^/!/" scm_code_ex.tmp2 > scm_code_ex.tmp3

    #the ^ means go to the beginning of the line and add the character !

#commenting out aggregation section lines 10-12
    sed -e "10s/^/!/" scm_code_ex.tmp3 > scm_code_ex.tmp4
    sed -e "11s/^/!/" scm_code_ex.tmp4 > scm_code_ex.tmp5
    sed -e "12s/^/!/" scm_code_ex.tmp5 > scm_code_ex.txt

#Remove unwanted files
rm *.tmp*

echo ""
echo "agg off ... auto off "
echo "Rebuild model now"
echo ""

elif [ "$agg" == 'on' ] && [ "$auto" == 'off' ]; then

#commenting out autoconversion section lines 2-5
    sed -e "3s/^/!/g" scm_code_ex.txt > scm_code_ex.tmp
    sed -e "4s/^/!/" scm_code_ex.tmp > scm_code_ex.tmp2
    sed -e "5s/^/!/" scm_code_ex.tmp2 > scm_code_ex.txt

#Remove unwanted files
rm *.tmp*

echo ""
echo "agg on ... auto off "
echo "Rebuild model now"
echo ""

elif [ "$agg" == 'off' ] && [ "$auto" == 'on' ]; then

#commenting out aggregation section lines 10-12

    sed -e "10s/^/!/" scm_code_ex.txt > scm_code_ex.tmp4
    sed -e "11s/^/!/" scm_code_ex.tmp4 > scm_code_ex.tmp5
    sed -e "12s/^/!/" scm_code_ex.tmp5 > scm_code_ex.txt

#Remove unwanted files
rm *.tmp*

echo ""
echo "agg off ... auto on"
echo "Rebuild model now"
echo ""
fi  

I'm very new to programming so I'm wondering how to make this better. For example, can sed be applied to a range of lines in one command? (I could not find something similar). To comment out lines 3-5, I tried sed "3,5{s/^/!/}" scm_code_ex.txt but that didn't work.

\$\endgroup\$
3
\$\begingroup\$

Since we're using Bash, we can combine the echo and read by using -p (prompt) argument to read like this:

read -p 'Enable aggregation [on/off] ' agg

I slightly adapted the form of the prompt to be more like common conventions from other interactive programs.

It's a good idea to validate the user input. If On is entered instead of on, then the user might be surprised to find it's equivalent to off!

unset agg
until [ "$agg" = on ] || [ "$agg" = off ]
do
    read -p 'Enable aggregation [ON/off] ' agg
    agg="${agg:-on}"
done

This also adds a default; that may or may not be appropriate here.


The multiple sed invocations could be combined into one:

sed -e "3s/^/!/" -e "4s/^/!/" -e "5s/^/!/" scm_code_ex.txt 

Since we want to perform the same operation on a range of lines, we can use an address range (3,5), and we can write the output file in place of the original (-i):

sed -i -e '3,5s/^/!/' scm_code_ex.txt

(The form you tried didn't work, because you need to separate the { from the enclosed command inside the group - but since there's only a single command, we don't need a group.)

We probably don't want to be doing the in-place editing: it's better to have a pristine (read-only) source file, and always generate the output file starting from there (we can even prefix it with a "DO NOT EDIT" comment). This makes it more amenable to generation by Make, as well as saving your bacon if you ever make a mistake in the script.


Instead of duplicating the commands for aggregation, one thing we can do is build the appropriate sed command using the options, then run it once. I would do this using Bash array variables:

replacements=()
[ "$auto" = on ] || replacements+=( -e '3,5s/^/!/' )
[ "$agg" = on ] || replacements+=( -e '10,12s/^/!/' )

sed -e "${replacements[@]}" scm_code_all.txt >scm_code.txt

Now we no longer need all those if/elif/fi blocks, and we no longer double the code size each time we add a new option.


The remaining fragile element in these scripts is the line-number addressing; this makes it hard to see which lines will be enabled or disabled, and it's hard to keep track when we add or remove lines from the source file. We can add some markers to this file (I'm assuming that ! begins a comment in that language):

code
code

!##BEGIN AUTO
code-auto
code-auto
code-auto
!##END AUTO

code
code

!##BEGIN AGG
code-agg
code-agg
!##END AGG

code
code

We can then use those lines for addressing:

[ "$auto" = on ] || replacements+=( -e '/^!##BEGIN AUTO/,/^!##END AUTO/s/^/!/' )
[ "$agg" = on ] || replacements+=( -e '/^!##BEGIN AGG/,/^!##END AGG/s/^/!/' )

We can reduce the amount of repetition if instead of using variables for auto and agg, we keep a list of features, and loop through them:

features=()
for i in auto agg
do
    while read -p "Enable '$i' feature? [YES/no] " f
    do case "${f,,}" in
           y|yes|on|true|'') features+=($i); break;;
           no|off|false) break;;
       esac
    done
done

replacements=()
for i in ${features[*]}
do
    replacements+=( -e "/^!##BEGIN ${i^^}/,/^!##END ${i^^}/s/^/!/" )
done

And we can combine the two loops into one, thus:

replacements=()
for i in auto agg
do
    while read -p "Enable '$i' feature? [YES/no] " f
    do case "${f,,}" in
           y|yes|on|true|'') 
               replacements+=( -e "/^!##BEGIN ${i^^}/,/^!##END ${i^^}/s/^/!/" );
               break;;
           n|no|off|false)
               break;;
       esac
    done
done

With these changes, we no longer need to remove any intermediate temporary files, but I'll note that the wildcard used is rather broad and may catch the user's own temporaries. Also, the temporaries are in the current working directory, not respecting $TMPDIR. To fix both of those, I prefer to create a temporary directory with mktemp -d, and trap the EXIT signal to remove all temporaries:

tmpdir=$(mktemp -d)
trap 'rm -r "$tmpdir"' EXIT

I hope this answer gives some food for thought. If you want a review of your code after considering the above, feel free to ask a new question with your amended code.

\$\endgroup\$
2
\$\begingroup\$

Yes, sed can be used for a range of lines, for example:

sed -e '3,5s/^/!/' scm_code_ex.txt

This inserts a ! at the start of line 3, 4, and 5.

I would write your script like this, and then I'll explain the details:

#!/bin/bash
# This script will comment in or out sections in the code text file

file=scm_code_ex.txt

read -p "- Aggregation (on) or (off) (type in ans)? " agg
read -p "- Autoconversion (on) or (off) (type in ans)? " auto

# Revert to original with everything ON.
sed -i.bak1 -e 's/^!//' "$file"

# Modifications according to conditions

if [ "$agg" = on ]; then
    echo
    echo "agg on"
    echo "Rebuild model now"
    echo
    sed -i.bak2 -e '/code-agg/s/^/!/' "$file"
fi

if [ "$auto" = on ]; then
    echo
    echo "auto on"
    echo "Rebuild model now"
    echo
    sed -i.bak3 -e '/code-auto/s/^/!/' "$file"
fi

rm "$file".bak?

I replaced the echo + read with read -p. The code is more compact, and I think it's nicer without the line break after the questions.

To revert the file to its original content, given your sample, it seems the ranges are not important, we can simply remove the ! from the beginning of all lines.

If the ranges are important, you could write like this:

sed -i.bak -e '3,5s/^!//' -e '10,12s/^!//' "$file"

I put the value "scm_code_ex.txt" into a variable, so that if I need to change it later, I can change it in one place in the code. It's right at the top, where it's easy to see.

I used the -i.bak flag of sed. This makes it perform the replacement in-place. It creates a backup file .bak. If needed, you can clean it up at the end. If it's not needed, then you can write like this instead if you are in Linux:

sed -i -e 's/^!//' "$file"

For the modifications, there's no need for all combinations of the agg and auto on/off states, they can be handled independently. The only downside is that this way the file may be written twice. If that's important to avoid, it's possible using a tip from @Toby's answer of building an array of the -e parameters:

commands=()

if [ "$agg" = on ]; then
    echo
    echo "agg on"
    echo "Rebuild model now"
    echo
    commands+=(-e '/code-agg/s/^/!/')
fi

if [ "$auto" = on ]; then
    echo
    echo "auto on"
    echo "Rebuild model now"
    echo
    commands+=(-e '/code-auto/s/^/!/')
fi

if [ ${#commands[@]} != 0 ]; then
    sed -i.bak "${commands[@]}" "$file"
fi

Finally, I did not bother with the ranges, but used the code-agg and code-auto patterns from your sample file. Again, if the ranges are important, you can update accordingly, for example replace /code-agg/ with 3,5:

sed -i.bak -e '3,5s/^/!/' "$file"
\$\endgroup\$
  • \$\begingroup\$ Did you mean to overwrite your initial backup when you run the second and third sed commands? I think it's better to use plain -i for those. Also, I think that the code-agg and code-auto in the sample input were probably intended to be meta rather than literal - although the question should have been much clearer about that. \$\endgroup\$ – Toby Speight Aug 30 '17 at 11:58
  • \$\begingroup\$ Strictly speaking 3,5 is an address range. 3 and 5 are the addresses (and it's permissible to mix line number and regex addressing to form a range, e.g. 1,/^END/. \$\endgroup\$ – Toby Speight Aug 30 '17 at 12:00
  • \$\begingroup\$ @TobySpeight The only I used -i.bak is to not have to explain the difference between GNU and BSD implementations (this works in both). Now I have to explain .bak, so I didn't gain much eh ;-) I thought I explained that I let the OP decide about the uses of 3,5, 10,12, code-agg and code-auto. Thanks for the clarification about terminology of address ranges, I fixed it (by removing my incorrect use). \$\endgroup\$ – Stop ongoing harm to Monica Aug 30 '17 at 12:08
  • \$\begingroup\$ You could -i.bak on the first replace (s/^!//), and then -i.bak2 or something on the others(/code-agg/s/^/!/ and /code-auto/s/^/!/) without needing reams of explanation, perhaps? That way, the user's original is preserved, and it's portable across different sed implementations. \$\endgroup\$ – Toby Speight Aug 30 '17 at 12:11

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

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