Can you give me some ideas on how to improve script?

Moltke · 2020-09-04 15:20:49

Hi everyone! Hope you're all having a nice life!

So, I took over practicing/learning bash scripting (once again ). I'm not very good at it and my knowledge is very basic. Last year I wrote a script to practice the use of if statements

if something_happens then do this else do this

what the script does/is about: cd to a dir, list files by extensions, if files with certain .ext exist, report they do, if they don't, report so as well. I used

ls

Can you take a look at it and tell me how and where to improve? I'm sure there may be several ways to do it I just can't figure it out by myself. FWIW, I've been reading and trying things but something's always wrong. Here it is:

#!/bin/bash
#listar archivos en disco USB, Seagate.
#Creador: Moltke
#Octubre 2019
#El propósito principal es aprender a usar condicionales en un script.
#Algunas variables que hacen el script portable y reutilizable
Seagate="/media/moltke/Seagate"
linea_blanca="\e[1;37m ++++++++++++++++++++++++\e[0m"
echo -e "$linea_blanca" 
if ( cd $Seagate && ls ./*.torrent &> /dev/null ); then   
  echo -e "\e[1;34m Archivos .torrent en Seagate:\e[0m" 
else 
  echo -e "\e[1;31m Ningún archivo .torrent en Seagate!\e[0m"
fi
#
echo -e "$linea_blanca" 
if ( cd $Seagate && ls ./*.mp3 &> /dev/null ); then 
   echo -e "\e[1;34m Archivos .mp3 en Seagate:\e[0m"
else
   echo -e "\e[1;31m Ningún archivo .mp3 en Seagate!\e[0m"
fi 
#
echo -e "$linea_blanca" 
if ( cd $Seagate && ls *.txt &> /dev/null ); then
   echo -e "\e[1;34m Archivos .txt en Seagate:\e[0m"
else 
   echo -e "\e[1;31m Ningún archivo .txt en Seagate!\e[0m"
fi 
#
echo -e "$linea_blanca" 
if ( cd $Seagate && ls *.png &> /dev/null ); then
   echo -e "\e[1;34m Archivos de imagen en Seagate:\e[0m"
else
   echo -e "\e[1;31m Ningun archivo de imagen en Seagate!\e[0m"
fi 
#
echo -e "$linea_blanca" 
if ( cd $Seagate && ls *.m??* &> /dev/null ); then
   echo -e "\e[1;34m Archivos de video en Seagate:\e[0m"
else 
   echo -e "\e[1;31m Ningún archivo de video en Seagate!\e[0m"
fi 
#
echo -e "$linea_blanca" 
if ( cd $Seagate && ls *.pdf &> /dev/null ); then
   echo -e "\e[1;34m Archivos pdf en Seagate:\e[0m"
else 
   echo -e "\e[1;31m Ningún archivo pdf en Seagate!\e[0m"
fi 
#
echo -e "$linea_blanca" 
if ( cd $Seagate && ls *.sh &> /dev/null ); then
   echo -e "\e[1;34m Archivos de script en Seagate:\e[0m"
else 
   echo -e "\e[1;31m Ningún archivo de script en Seagate!\e[0m"
fi 
#
echo -e "$linea_blanca" 
if ( cd $Seagate && ls *.iso &> /dev/null ); then
   echo -e "\e[1;34m Archivos iso en Seagate:\e[0m"
else 
   echo -e "\e[1;31m Ningún archivo iso en Seagate!\e[0m"
fi 
#
echo -e "$linea_blanca" 
if ( cd $Seagate && ls *.zip &> /dev/null ); then
   echo -e "\e[1;34m Archivos zip en Seagate:\e[0m"
else 
   echo -e "\e[1;31m Ningún archivo zip en Seagate!\e[0m"
fi 
#
echo -e "$linea_blanca" 
if ( cd $Seagate && ls *.aria2 &> /dev/null ); then
   echo -e "\e[1;34m Archivos de aria2 en Seagate:\e[0m"
else 
   echo -e "\e[1;31m Ningún archivo de aria2 en Seagate!\e[0m"
fi 
#
echo -e "$linea_blanca" 
if ( cd $Seagate && ls *.srt &> /dev/null ); then
   echo -e "\e[1;34m Archivos de subtitulos en Seagate:\e[0m"
else 
   echo -e "\e[1;31m Ningún archivo de subtitulos en Seagate!\e[0m"
fi

Thanks in advance for all your answers!

NOTE: My native language's Spanish so that's what I use in my scripts for comments and messages so here's what the lines in that language above say:

1. - $linea_blanca = $white_line

2. - Archivos .ext en Seagate = Files with .ext in Seagate

3. - Ningún archivo con .ext en Seagate! = No files with .ext in Seagate!

4. - listar archivos en disco USB, Seagate = list files in USB disk, Seagate.

5. - Creador: Moltke = Creator: Moltke

6. - Octubre 2019 = October 2019

7. - El propósito principal es aprender a usar condicionales en un script = the main purpose is to learn how to use conditional statements in a script.

8. - Algunas variables que hacen el script portable y reutilizable = some variables to make the script portable and reusable.

HINT: What I'm asking for is something like "you can do this keyword see here some_link I'm not asking for specific "hacks" though they're very welcome hope this makes any sense to you and sorry in advance if it doesn't.

Last edited by Moltke (2020-09-04 15:31:12)

Trilby · 2020-09-04 15:38:18

A first improvement would be to use a loop to drastically simplify the whole script while making it easier to add/remove file types:

Seagate="/media/moltke/Seagate"
linea_blanca="\e[1;37m ++++++++++++++++++++++++\e[0m"

extensions=".torrent .mp3 .txt .png .m?? .pdf .sh .iso .zip .aria2 .srt"

for extension in $extensions; do
   echo -e $linea_blanca
   if ls $Seagate/*$extension >/dev/null 2>&1; then
      echo -e "\e[1;34m Archivos de $extension en Seagate:\e[0m"
   else
      echo -e "\e[1;31m Ningún archivo de $extension en Seagate!\e[0m"
   fi
done

But a much better way to achieve similar information would be to run `file` on the contents of the directory just to see what filetypes are there (and not rely on extensions):

#!/bin/sh

Seagate="/media/moltke/Seagate"

echo "Seagate contains the following file types:"
file -b --mime-type $Seagate/* | sort -u

Of course, this doesn't help you practice using 'if' conditionals - but I just don't think that's a good way to approach the task at hand.

Last edited by Trilby (2020-09-04 18:10:00)

Moltke · 2020-09-04 18:00:26

Trilby wrote:

A first improvement would be to use a loop to drastically simplify the whole script while making it easier to add/remove file types:
Seagate="/media/moltke/Seagate"
linea_blanca="\e[1;37m ++++++++++++++++++++++++\e[0m"

extensions=".torrent .mp3 .txt .png .m?? .pdf .sh .iso .zip .aria2 .srt"

for extension in extensions; do
   echo $linea_blanca
   if ls $Segate/*.$extension >/dev/null 2>&1; then  -----> Here's a typo, it should read Seagate not Segate :)
      echo -e "\e[1;34m Archivos de $extension en Seagate:\e[0m"
   else
      echo -e "\e[1;31m Ningún archivo de $extension en Seagate!\e[0m" -----> Here's another typo, a "s" at the end of extension's missing, it reads "extension" when should be "extensions" :)
   fi
done
But a much better way to achieve similar information would be to run `file` on the contents of the directory just to see what filetypes are there (and not rely on extensions):
#!/bin/sh

Seagate="/media/moltke/Seagate"

echo "Seagate contains the following file types:"
file -b --mime-type $Seagate/* | sort -u
Of course, this doesn't help you practice using 'if' conditionals - but I just don't think that's a good way to approach the task at hand.

Thanks for your reply. Your suggestions to modify the script don't work as expected; files are not being listed, they're there though. At first I thought it was to due to a couple of typos you made and didn't catch; you wrote Segate instead of Seagate and you missed a "s" at the of extension in the echo line, since I copied/pasted your code it was like that, when I realized it I corrected it, but still got:

No files with .ext in Seagate

but if run the original version it does list the files. The second one does a fine job, thanks for sharing, however, I'd prefer that it lists file's names too not file's types only so I can copy the name and do something with it, i.e move it, copy it, remove it, rename it and so on.

Trilby · 2020-09-04 18:09:04

Sorry about that, I fixed a couple typos and tested it this time (I had to change the "Seagate" variable to test it).

Moltke · 2020-09-11 17:31:33

Trilby wrote:

Sorry about that, I fixed a couple typos and tested it this time (I had to change the "Seagate" variable to test it).

I didn't come with this all by myself, someone else helped me, it does what I want, however, I have to deal with spaces in file names so the output's a bit cleaner but learnt a lot with this exercise and think I understand a little bit more what people https://unix.stackexchange.com/question … do-instead are talking about.

#!/usr/bin/env bash
seagate="/media/moltke/Seagate"
linea_blanca="\e[1;37m ++++++++++++++++++++++++\e[0m"

while IFS="| " read -ra arr; do
    echo -e "$linea_blanca"
    foundFiles="$(ls -1 "$seagate"/${arr[0]} 2> /dev/null)"
    if [ -z "$foundFiles" ] 
    then
        echo -e "\e[1;31m Ningún archivo ${arr[1]} en seagate:\e[0m"
    else
        echo -e "\e[1;34m Archivos ${arr[1]} en seagate:\e[0m"
        echo -e "$foundFiles" | xargs -n 1  basename 
    fi
done << EOM
*.torrent| torrent
*.mp3| mp3
*.txt| txt
*.png| png
*.m??*| video
*.pdf| pdf
*.sh| shellscript
*.iso| iso
*.zip| zip
*.aria2| aria2
*.srt| subtitulos
EOM

Trilby · 2020-09-11 18:43:34

I still don't think looping through every option and generating a new list of files each time is a good approach. List all files, and sort it into groups, then display. This also uses `file` to sort all filetypes:

#!/bin/sh

seagate="/media/moltke/Seagate"
cd "$seagate"

file --mime-type * | awk '
   {
      sub(/:/,"",$1)
      files[$2]=files[$2]"\n\t"$1
   }
   END {
      for (key in files)
         printf "\n\e[1;37m ++++++++++++++++++++++++\n\e[1;34m Archivos de %s en Seagate:\e[0m%s", key, files[key]
      print ""
   }
'

But if you really just want to stick with that list, and want to loop and say when no files are present rather than sort, you still don't need a special IFS or bash arrays:

#!/bin/sh

seagate="/media/moltke/Seagate"
linea_blanca="\e[1;37m ++++++++++++++++++++++++\e[0m"

cd "$seagate"

while read glob description; do
    echo -e "$linea_blanca"
    if ls $glob >/dev/null 2>&1; then
        echo -e "\e[1;34m Archivos $description en seagate:\e[0m"
        ls -1 $glob
    else
        echo -e "\e[1;31m Ningún archivo $description en seagate:\e[0m"
    fi
done << EOM
*.torrent torrent
*.mp3 mp3
*.txt txt
*.png png
*.m??* video
*.pdf pdf
*.sh shellscript
*.iso iso
*.zip zip
*.aria2 aria2
*.srt subtitulos
EOM

Last edited by Trilby (2020-09-11 18:54:18)

Moltke · 2020-09-11 20:35:15

Trilby wrote:

I still don't think looping through every option and generating a new list of files each time is a good approach. List all files, and sort it into groups, then display. This also uses `file` to sort all filetypes:
#!/bin/sh

seagate="/media/moltke/Seagate"
cd "$seagate"

file --mime-type * | awk '
   {
      sub(/:/,"",$1)
      files[$2]=files[$2]"\n\t"$1
   }
   END {
      for (key in files)
         printf "\n\e[1;37m ++++++++++++++++++++++++\n\e[1;34m Archivos de %s en Seagate:\e[0m%s", key, files[key]
      print ""
   }
'
But if you really just want to stick with that list, and want to loop and say when no files are present rather than sort, you still don't need a special IFS or bash arrays:
#!/bin/sh

seagate="/media/moltke/Seagate"
linea_blanca="\e[1;37m ++++++++++++++++++++++++\e[0m"

cd "$seagate"

while read glob description; do
    echo -e "$linea_blanca"
    if ls $glob >/dev/null 2>&1; then
        echo -e "\e[1;34m Archivos $description en seagate:\e[0m"
        ls -1 $glob
    else
        echo -e "\e[1;31m Ningún archivo $description en seagate:\e[0m"
    fi
done << EOM
*.torrent torrent
*.mp3 mp3
*.txt txt
*.png png
*.m??* video
*.pdf pdf
*.sh shellscript
*.iso iso
*.zip zip
*.aria2 aria2
*.srt subtitulos
EOM

Fisrt one doesn't work, got this:

Syntax error: Unterminated quoted string

. I copied/pasted the whole thing so not sure where the syntax error may be. Second one works great! It even deals with spaces in file names. Thanks!

EDIT: Just noticed you used /bin/sh, I changed that to /bin/bash and the syntax error's gone, however, still doesn't work. Also,

Last edited by Moltke (2020-09-11 20:46:03)

Trilby · 2020-09-11 20:39:02

There is no need to "deal with" spaces in filenames if you don't parse the output of `ls` which you really should avoid doing anyways.

As for the first one, I ran it here and it works fine. There's definitely not a mismatching quote unless you failed to copy the last line of it with just the closing single-quote.

Moltke · 2020-09-11 20:51:46

Trilby wrote:

There is no need to "deal with" spaces in filenames if you don't parse the output of `ls` which you really should avoid doing anyways.
As for the first one, I ran it here and it works fine. There's definitely not a mismatching quote unless you failed to copy the last line of it with just the closing single-quote.

Nope, I copied/pasted the code just fine but it doesn't work for me. So I've read, that one should avoid parsing 'ls' output and I'm starting to understand why.

2ManyDogs · 2020-09-11 22:34:35

Why are you also asking this question on the Debian forum? Are you using Arch, or Debian?

Moltke · 2020-09-12 00:18:22

2ManyDogs wrote:

Why are you also asking this question on the Debian forum? Are you using Arch, or Debian?

Both. And the question isn't specific either one of them so I thought I could ask, was I wrong? If so, my apologies.

Arch Linux

#1 2020-09-04 15:20:49

Can you give me some ideas on how to improve script?

#2 2020-09-04 15:38:18

Re: Can you give me some ideas on how to improve script?

#3 2020-09-04 18:00:26

Re: Can you give me some ideas on how to improve script?

#4 2020-09-04 18:09:04

Re: Can you give me some ideas on how to improve script?

#5 2020-09-11 17:31:33

Re: Can you give me some ideas on how to improve script?

#6 2020-09-11 18:43:34

Re: Can you give me some ideas on how to improve script?

#7 2020-09-11 20:35:15

Re: Can you give me some ideas on how to improve script?

#8 2020-09-11 20:39:02

Re: Can you give me some ideas on how to improve script?

#9 2020-09-11 20:51:46

Re: Can you give me some ideas on how to improve script?

#10 2020-09-11 22:34:35

Re: Can you give me some ideas on how to improve script?

#11 2020-09-12 00:18:22

Re: Can you give me some ideas on how to improve script?

Board footer