2
\$\begingroup\$

I wrote a bash script for reviewing scientific articles. It adds a blank page for notes and creates a new landscape file. I'm sure it can be made better code wise. Any suggestions?

#!/bin/bash

if [ $# -ne 1 ]
then
  echo "Usage example: ./bashscript src.pdf"
  exit $E_BADARGS
else
  NUM=$(pdftk $1 dump_data | grep 'NumberOfPages' | awk '{split($0,a,": "); print a[2]}')
  COMMSTR=''

  for i in $(seq 1 $NUM);
  do
    COMMSTR="$COMMSTR A$i B1 " 
  done
  $(echo "" | ps2pdf -sPAPERSIZE=a4 - pageblanche.pdf)
  $(pdftk A=$1 B=pageblanche.pdf cat $COMMSTR output 'mod_'$1)
  (pdfnup 'mod_'$1 --nup 2x1 --landscape --outfile 'print_'$1)
  $(rm pageblanche.pdf && rm 'mod_'$1)

fi

#for f in *.pdf; do ./bashscript.sh $f; done 2> /dev/null
\$\endgroup\$

1 Answer 1

1
\$\begingroup\$

First of all "bashscript" is a very poor name:

  • It doesn't tell what the script does. "prepare-articles-for-review.sh" would be better. Or just "prepare-articles.sh"
  • It is customer to use .sh extension for Bash scripts

If your typical use case is this:

for f in *.pdf; do ./bashscript.sh $f; done 2> /dev/null

Then it would be better to make the script handle multiple file parameters.


  exit $E_BADARGS

I'm not familiar with such variable. I don't think it's a standard, and it's not mentioned in my man bash. If the variable is undefined (and I think it is), then this will have the same effect as exit, which is equivalent to exit $?, where $? is the exit code of the last statement, in this case the echo on the previous line, which is most probably 0. I think you intended to exit with failure, with non-zero exit code.


  NUM=$(pdftk $1 dump_data | grep 'NumberOfPages' | awk '{split($0,a,": "); print a[2]}')

Many problems here:

  • You should double-quote "$1", to protect from spaces in the filename
  • The quoting in grep 'NumberOfPages' is redundant
  • Are you sure there won't be more than one lines matching "NumberOfPages"? Just to be safe, I would add exit in the Awk command
  • The Awk command can be written simpler as awk -v FS=": " '{print $2}'

  COMMSTR=''

You can simplify to:

  COMMSTR=

  for i in $(seq 1 $NUM);

seq is not recommended because it's not portable. You can achieve the same using native Bash functionality:

for ((i=1; i<=$NUM; i++));

  $(echo "" | ps2pdf -sPAPERSIZE=a4 - pageblanche.pdf)

The $(...) wrapping is really pointless, and awful. No need for echo "", simply echo is exactly the same.


  $(pdftk A=$1 B=pageblanche.pdf cat $COMMSTR output 'mod_'$1)

Does this work at all? Unfortunately I don't have pdftk so cannot try. Maybe it's fine, but it looks suspicious.

Btw, you don't need the quotes in 'mod_'$1. And again, the $(...) wrapping is pointless.


  (pdfnup 'mod_'$1 --nup 2x1 --landscape --outfile 'print_'$1)

As earlier, you don't need those quotes, and the (...) wrapping is pointless.

The same goes for the rest of the code.

Suggested implementation

I cannot fully test this because I don't have pdftk, but it should work:

#!/bin/bash

if [ $# = 0 ]
then
  echo "Usage: $0 file1.pdf file2.pdf ..."
  exit 1
fi

for file; do
    NUM=$(pdftk "$file" dump_data | awk -v FS=": " '/NumberOfPages/ { print $2; exit }')
    COMMSTR=

    for ((i=1; i<=$NUM; i++)); do COMMSTR="$COMMSTR A$i B1 "; done

    blank=blank.pdf
    ps2pdf -sPAPERSIZE=a4 - $blank < /dev/null
    pdftk A="$file" B=$blank cat $COMMSTR output mod_"$file"
    pdfnup mod_"$file" --nup 2x1 --landscape --outfile print_"$file"
    rm $blank && rm mod_"$file"
done
\$\endgroup\$
3
  • 1
    \$\begingroup\$ Works perfectly. Thank you for your time and work. \$\endgroup\$
    – ucyo
    Commented Oct 31, 2014 at 21:32
  • \$\begingroup\$ exit is not equivalent to exit 0 it is equivalent to exit $?. \$\endgroup\$ Commented Nov 4, 2014 at 2:06
  • \$\begingroup\$ @EtanReisner well spotted! I clarified that point now. Thanks for the review-review, as usual ;-) \$\endgroup\$
    – janos
    Commented Nov 4, 2014 at 9:00

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.