You are not logged in.

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

Moltke
Member
Registered: 2016-07-12
Posts: 58

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

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

So, I took over practicing/learning bash scripting (once again big_smile). 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! smile

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 smile 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)


Judges have decimated, meanings of our laws, already guilt? something has a flow, wrongly accused, blindly confused, open your eyes, wake up realize!

Offline

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

Trilby
Inspector Parrot
Registered: 2011-11-29
Posts: 24,240
Website

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

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)


"UNIX is simple and coherent..." - Dennis Ritchie, "GNU's Not UNIX" -  Richard Stallman

Offline

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

Moltke
Member
Registered: 2016-07-12
Posts: 58

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

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.


Judges have decimated, meanings of our laws, already guilt? something has a flow, wrongly accused, blindly confused, open your eyes, wake up realize!

Offline

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

Trilby
Inspector Parrot
Registered: 2011-11-29
Posts: 24,240
Website

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

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


"UNIX is simple and coherent..." - Dennis Ritchie, "GNU's Not UNIX" -  Richard Stallman

Offline

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

Moltke
Member
Registered: 2016-07-12
Posts: 58

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

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. smile

#!/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

Judges have decimated, meanings of our laws, already guilt? something has a flow, wrongly accused, blindly confused, open your eyes, wake up realize!

Offline

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

Trilby
Inspector Parrot
Registered: 2011-11-29
Posts: 24,240
Website

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

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)


"UNIX is simple and coherent..." - Dennis Ritchie, "GNU's Not UNIX" -  Richard Stallman

Offline

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

Moltke
Member
Registered: 2016-07-12
Posts: 58

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

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! smile

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)


Judges have decimated, meanings of our laws, already guilt? something has a flow, wrongly accused, blindly confused, open your eyes, wake up realize!

Offline

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

Trilby
Inspector Parrot
Registered: 2011-11-29
Posts: 24,240
Website

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

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.


"UNIX is simple and coherent..." - Dennis Ritchie, "GNU's Not UNIX" -  Richard Stallman

Offline

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

Moltke
Member
Registered: 2016-07-12
Posts: 58

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

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.


Judges have decimated, meanings of our laws, already guilt? something has a flow, wrongly accused, blindly confused, open your eyes, wake up realize!

Offline

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

2ManyDogs
Forum Moderator
Registered: 2012-01-15
Posts: 3,336

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

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

Offline

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

Moltke
Member
Registered: 2016-07-12
Posts: 58

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

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.


Judges have decimated, meanings of our laws, already guilt? something has a flow, wrongly accused, blindly confused, open your eyes, wake up realize!

Offline

Board footer

Powered by FluxBB