4
\$\begingroup\$

This produces recursive directory listings(it's intended to do other things, so there will be something else instead of echo, but it isn't important), yet it isn't very intuitive. How can I improve it?

#! /bin/bash
deploy ()
{
    for file in $(ls $1); do
        if [ ! -d "$2$file" ]; then
            echo ${2}${file}
        else
            deploy $2$file "$2$file/"
        fi
    done;
}

deploy .

Output example:

tmp/kdecache-z/http/ba9c27404980248a61dcdadd35f5f52395b8bdde
tmp/kdecache-z/http/scoreboard
tmp/kdecache-z/icon-cache.kcache
tmp/kdecache-z/ksycoca4
tmp/kdecache-z/ksycoca4stamp
tmp/kdecache-z/plasma-svgelements-.customized1
tmp/kdecache-z/plasma-svgelements-internal-system-colors
tmp/kdecache-z/plasma_theme_.customized1.kcache
tmp/kdecache-z/plasma_theme_internal-system-colors.kcache
tmp/kdecache-z/Активные
tmp/kdecache-z/напоминанияrc
tmp/kdecache-z/Устаревшие
tmp/kdecache-z/напоминанияrc
tmp/kdecache-z/Шаблоны
tmp/kdecache-z/напоминанийrc
tmp/kde-z/xauth-1000-_0
tmp/ksocket-z/kdeinit4__0
tmp/ksocket-z/kio_http_cache_cleaner
tmp/ksocket-z/klauncherhX2255.slave-socket
tmp/ksocket-z/KSMserver__0
tmp/pulse-PKdhtXMmr18n/native
tmp/pulse-PKdhtXMmr18n/pid
tmp/qtsingleapp-homezS-8560-3e8
tmp/qtsingleapp-homezS-8560-3e8-lockfile
tmp/skype-2664/DbTemp./temp-KXp68f252Xx0kBMtqNdj7NFo
tmp/skype-2664/DbTemp./temp-NAailvY24hLGYNrZiJ99QIgH
tmp/sni-qt_skype_2664-v6Xyna/icons/hicolor/24x24/apps/skype_2664_2d1ee5482260fd9cd180b32787792683.png
tmp/sni-qt_skype_2664-v6Xyna/icons/hicolor/24x24/apps/skype_2664_37e170fc54e7355d9d298917e74f9ea9.png
tmp/sni-qt_skype_2664-v6Xyna/icons/hicolor/24x24/apps/skype_2664_602c84fa0f4d61c64f770495a500279e.png
tmp/sni-qt_skype_2664-v6Xyna/icons/hicolor/24x24/apps/skype_2664_f2fc4a539a7b9553f5b35241d1154e84.png
tmp/z.socket

P.S. It is working perfectly for me so I just want to make it more beautiful.

\$\endgroup\$
  • 1
    \$\begingroup\$ This can break if there's a directory with a space in the name. \$\endgroup\$ – choroba Feb 16 '15 at 0:38
  • \$\begingroup\$ @choroba it isn't very important, but if this can be fixed i'll be happy \$\endgroup\$ – Zhigalin Feb 16 '15 at 0:46
  • \$\begingroup\$ This seems to be a very specialized kind of directory listing. Could you edit the question to add more detail about what the code is intended to do, and why you need such a function? \$\endgroup\$ – 200_success Feb 16 '15 at 6:38
  • \$\begingroup\$ Btw. depending on what you use this for a combination of find and xargs might be easier/faster. \$\endgroup\$ – ferada Feb 16 '15 at 11:14
3
\$\begingroup\$

In fairness, this task is relatively simple, and your solution is actually pretty good.

Shell scripts are often just a hack to solve a specific task, and, your script is much better than many that I see that do that.

There are two things to consider in your script. The first is whether it can be improved. The second is whether there's a better way.

Now, about improving it. I would:

  • remove the need for the second parameter
  • name the parameters,
  • include the / in a different place
  • use quotes around variables in specific places.
  • reverse the if-blocks to get rid of the 'negate' condition.

Consider the following:

deploy () {
    local dir="$1"
    for file in $(ls "$dir/" ); do
        # Fully qualified file
        fqfile="$dir/$file"
        if [ -d "$fqfile" ]; then
            deploy "$fqfile"
        else
            echo "$fqfile"
        fi
    done;
}

Introducing the fqfile allows the simplifcation of other variables too.

Now, having said all that, your command could easily be solved with the find command:

find . -type f -exec echo {} \;

The above command will recursively find all files below ., and echo them.

\$\endgroup\$
  • \$\begingroup\$ Why not just omit the -exec altogether? \$\endgroup\$ – janos Feb 16 '15 at 12:42
  • \$\begingroup\$ @janos - because the original question indicated they wanted to do more than just echo the output \$\endgroup\$ – rolfl Feb 16 '15 at 12:42
  • \$\begingroup\$ how I should launch your function? \$\endgroup\$ – Zhigalin Feb 17 '15 at 22:37
  • \$\begingroup\$ @Zhigalin - which function. The 'find' alternative takes the directory to 'deploy' as the first argument (like find . -type .... or find /some/dir -type f ...). The alternative deploy() function I suggested will be called the same way as your code, i.e. deploy . (or deploy /some/dir) \$\endgroup\$ – rolfl Feb 17 '15 at 22:39
  • \$\begingroup\$ "Find" is not a function)) So, we're speaking of deploy() If I'll call it with $1='.' the listing will look like ./foo/bar ./foobar ./bar/xyz If I'll cal it with $1='/some/dir' the listing will look like /some/dir/foo/bar /some/dir/foobar /some/dir/bar/xyz I can get rid of this with with ${fqfile#./}, but it seems to be an ugly hack. P.S. "find" is not suited for my use because I'll extend the script in the future. P.P.S. my code is working(there isn't any strange\ files in my projects), the question was only about the aesthetic aspect of my script . \$\endgroup\$ – Zhigalin Feb 17 '15 at 23:00
4
\$\begingroup\$

This on top of what @rolfl already said.

It's a bad practice to parse the output of ls. Iterating over the output of $(ls ...) falls under that category. For example, the script will not work for files with spaces in the name.

A better way would be using standard shell expansion, like this:

#!/bin/bash
deploy() {
    local dir="$1"
    for path in "$dir"/*; do
        if [ -d "$path" ]; then
            deploy "$path"
        elif [ -f "$path" ]; then
            echo "$path"
        fi
    done
}

Note that I had to add an extra elif, otherwise empty directories would be included too. But this extra condition is probably a good thing, as it seems you want to work with files anyway, and this code makes that point explicit.

Lastly, some minor coding style issues:

  • Don't put a semicolon at the end lines (like you did in done;)
  • Don't put a space after #! in the shebang line
  • The common way to write function declarations is fname() { rather than fname () and a brace on the next line
\$\endgroup\$
  • \$\begingroup\$ how I should launch your function? \$\endgroup\$ – Zhigalin Feb 17 '15 at 22:37
  • \$\begingroup\$ @Zhigalin the same way as your original, deploy ., why do you ask? \$\endgroup\$ – janos Feb 17 '15 at 22:58
  • \$\begingroup\$ see my comment on rolfl's answer \$\endgroup\$ – Zhigalin Feb 17 '15 at 23:02

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.