4
\$\begingroup\$

I am writing a shell script to sort my files by extension and unzip archives. It works in simple cases, but I haven't tested it extensively. How can I make it more robust against the set of file names that may be present?

#!/bin/bash
#File sorter
SECONDS=0
SAVEIFS=$IFS
IFS=$(echo -en "\n\b")
echo "trigger took place in $(pwd)"
for i in $( ls -1); do
  echo "processing $i ..."
  fileext=${i##*.}
  case ${i##*.} in
    zip)
      mkdir archive
      mkdir $(pwd)/archive/${i%%.*}
      unzip $i -d $(pwd)/archive/${i%%.*}
      ;;
    *) echo "nonzip"
      mkdir ${i##*.}
      mv $i $(pwd)/${i##*.}
      ;;
  esac
done
echo "Done in $SECONDS seconds!"
\$\endgroup\$
3
  • \$\begingroup\$ Given that two of us have posted a review of your code, I'm going to suggest that this question be migrated to Code Review. Do not repost there! Once you've figured out what you wanted to do with ask, please ask a new question here on Unix & Linux. \$\endgroup\$ Mar 3, 2012 at 1:18
  • \$\begingroup\$ I've edited your question to make it fit the CR format better, because I think that's what would benefit you most at this point. Please do come back here on Unix & Linux to ask about that report in awk. \$\endgroup\$ Mar 3, 2012 at 2:20
  • \$\begingroup\$ @Gilles I wanted to ask how I can generate report. So you mean, I should not attach my codes since that would be counted as code review? I attached the codes so I would give more explanation on what I actually do. But thank, I have got some really good comments about the codes. \$\endgroup\$
    – Erfan
    Mar 4, 2012 at 0:45

2 Answers 2

7
\$\begingroup\$

I don't see where awk fits in this. I'm going to point out a few problems in your script, so you'll at least get something useful out of this. But please do tell us what you're actually trying to do.

IFS=$(echo -en "\n\b")

There's a simpler way of writing that in bash: IFS=$'\n\b'. I don't understand why you're setting $IFS to include a backspace character; and whatever need you may have found to set IFS is probably caused by other problems that I'll point out below.

echo "trigger took place in $(pwd)"

There's a variable $PWD that contains the current working directory. $PWD is equivalent to $pwd and a little faster.

for i in $( ls -1); do

Do not parse the output of ls. ls is a tool for displaying the attributes of files. The shell has a perfectly good construct for listing files in a directory: wildcards. Make this for i in *; do … This way has the advantage that it can cope with arbitrary file names; parsing the output of ls will always choke on some “strange” file names.

fileext=${i##*.}

You should use this variable below.

mkdir archive

If you run this part multiple times, the directory will already exist. Test for the directory's existence first, or call mkdir -p archive.

mkdir $(pwd)/archive/${i%%.*}

Again, $PWD. Except that there's no point in using it, a relative path would do. Also, you need double quotes around all variable and command substitutions; otherwise the shell splits the result of the substitution at $IFS characters, then interprets the pieces as glob patterns (file name wildcards) and expands them. Always put double quotes around variable and command substitutions unless you know why you need to leave them off.

Another point: ${i%%.*} strips everything after the first . in the file name. This is not compatible with the definition of $fileext. To get the file except for its last extension, use ${%.*} (truncate at the last .).

$(unzip $i -d $(pwd)/archive/${i%%.*})

I don't know what you're trying to do here, interpreting the output of unzip as a shell snippet. Again, double quotes around variable substitutions.

  mkdir ${i##*.}

Again, double quotes. Also, if the file name sans prefix happens to begin with a -, the mkdir command will interpret its argument as an option. To avoid this, use -- to indicate the end of the option list (this is a convention that most utilities respect): mkdir -- "${i##*.}"

Also, there's a case you don't handle: what if the file name contains no .? In that case, mkdir will attempt to overwrite the file (and fail). One solution is to create the directory with a temporary name, move the file, then rename it.

Here's the fixed code:

#!/bin/bash
#File sorter
set -e    # Abort in case of error
SECONDS=0
SAVEIFS=$IFS
echo "trigger took place in $PWD"
for i in *; do
  echo "processing $i ..."
  case $i in
    *.zip)
      [ -d archive ] || mkdir archive
      mkdir "archive/${i%.*}"
      unzip -d "archive/${i%.*}" "./$i";;
    *.*)
      mkdir -p "${i##*.}"
      mv -- "$i" "${i##*.}/";;
    *)
      tmp=$(TMPDIR=. mktemp -d)
      mv -- "$i" "$tmp/"
      mv -- "$tmp" "$i";;
  esac
done
echo "Done in $SECONDS seconds!"
\$\endgroup\$
6
  • \$\begingroup\$ P.S. I haven't tested the resulting code, there may be remaining bugs. \$\endgroup\$ Mar 3, 2012 at 1:28
  • \$\begingroup\$ +1 nice explanation. You don't need the SAVEIFS variable now that you no longer change IFS. \$\endgroup\$ Mar 3, 2012 at 17:23
  • \$\begingroup\$ @Gilles WOW. Thank man I really really appreciate what you did. Let me clarify what I want to do. I want to make directory for every file's extension that is present in $PWD and move the file there. Except if it is a zip file I want to extract it to the directory archive. I needed to overwrite $IFS since the filenames with spaces was counted as multiple files in the loop. And I needed to generate a report of what file moved/extracted where after it finished its process. That is why I wanted to use AWK. I hope I explained well what I want to do. Thank you really much for what you said so far. \$\endgroup\$
    – Erfan
    Mar 4, 2012 at 0:55
  • \$\begingroup\$ @Erfan Please post a new question on Unix & Linux explaining what you want in that report. What you wrote in your comment here is a good start; an example report for a couple of input files would help a lot. \$\endgroup\$ Mar 4, 2012 at 1:08
  • \$\begingroup\$ @Gilles Thanks man. I actually got what I need just a simple text report and I made it. I just have only 1 problem and I really do not have a clue how to do it. I want to pass command arguments along with my shell script. For example the user would be able to write sh fsorter.sh -d /Downloads and the script runs in Downloads. Or some other ones. Can you help me understand where to start? \$\endgroup\$
    – Erfan
    Mar 4, 2012 at 1:27
2
\$\begingroup\$

I don't see why you want to use awk, or what kind of report you want to see. I have made some code-review-type adjustments to your script:

#!/bin/bash
# sort files into subdirectories
start=$SECONDS                    # this is a magic bash variable
echo "trigger took place in $PWD"
for file in *; do                 # don't need to call ls
  echo "processing $file ..."
  fileext=${file##*.}             # if you define a variable, you might as well use it
  case $fileext in
  zip)
    dirname=archive/${file%.*}
    mkdir -p "$dirname"           # make the parent and the subdir in one call
    unzip "$file" -d "$dirname"
    ;;
  *)
    echo "nonzip"
    mkdir -p "$fileext"           # -p option suppresses error message if dir exists
    mv "$file" "$fileext"
    ;;
  esac
done
echo "Done in $((SECONDS-start)) seconds!"

If you have any questions, please add a comment.

\$\endgroup\$
1
  • \$\begingroup\$ Looks like we had the same leaning. I caught a few more bugs. SECONDS would work as intended in the original, it can be assigned to. \$\endgroup\$ Mar 3, 2012 at 1:27

Your Answer

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

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