How to improve script for redacting and copying a file

Question

I have recently started bash scripting, with little prior coding experience, and want to see how the below code could be cleaner/improved.

This is a subsection of a bigger script, but the point of it is to redact '42' from a file (secretinfo.md) and replace it with XX and then put the file in the new location. I don't want to delete the original file.

$files is defined earlier as the variable for a for-loop going through my target directory.

if [ "$files" == "source/secretinfo.md" ]
then
  echo $files "is being redacted."
  cd source/
  cp secretinfo.md secretinfo_redacted.md
sed -i 's/42/XX/g' secretinfo_redacted.md
  mv secretinfo_redacted.md ../build/
echo $files "has been copied."
cd ..
else
  echo $files "is being copied into build for you."
  cp $files build/.
fi
done

Thanks for any tips or tricks.

How is $files defined? Is it supposed to be one file or many? Is it a string or an array? What is the for loop? — terdon, Oct 19 '23 at 12:27
Welcome. As tip, you can use shellcheck. You can also install it in your OS and there is a VScode extension. — schrodingerscatcuriosity, Oct 19 '23 at 14:27

score 4 · Answer 1 · edited Oct 19 '23 at 13:18

First off, always quote your variables and this goes double if they store file names, since they can have any character except NUL (\0) and /. You're also doing some unnecessary movement and creation of temporary files, and you don't have any error checking, so even if one of the steps fails, the others will continue and might cause problems. Next, as a general rule printf is a better choice than echo. And finally, since $files seems to be designed to hold a single file, you might as well name it $file. It is helpful to have variable names that are semantically consistent since that will help you understand your code when you come back to it after a few years.

Try this:

for file in source/*; do
  if [ "$file" = "source/secretinfo.md" ]
  then
    printf '%s is being redacted.\n' "$file"
    sed 's/42/XX/g' -- "$file" > build/secretinfo_redacted.md &&
    printf '%s has been copied.\n' "$file" ||
      printf 'An error occurred, %s has not been copied.\n' "$file"
  else
    printf '%s is being copied into build for you.\n' "$file"
    cp -- "$file" build/ || 
       printf 'An error occurred, %s has not been copied.\n' "$file"
  fi
done

Thanks for this, useful breakdown and shows a lot of interesting things I wasn't aware of. Such as using || within the code. — RunRenegade, Oct 31 '23 at 08:20

How to improve script for redacting and copying a file

1 Answers1