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

[Moltke]

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

Code: Select all

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

Code: Select all

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:

Code: Select all

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

  cd $Seagate && ls *.torrent

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"

   cd $Seagate && ls *.mp3

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"

   cd $Seagate && ls *.txt

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"

   cd $Seagate && ls *.png

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"

   cd $Seagate && ls *.m??*

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"

   cd $Seagate && ls *.pdf

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"

   cd $Seagate && ls *.sh

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"

   cd $Seagate && ls *.iso

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"

   cd $Seagate && ls *.zip

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"

   cd $Seagate && ls *.aria2

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"

   cd $Seagate && ls *.srt

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

[Head_on_a_Stick]

https://www.shellcheck.net/

EDIT: see also https://mywiki.wooledge.org/ParsingLs

[Moltke]

https://www.shellcheck.net/

EDIT: see also https://mywiki.wooledge.org/ParsingLs

Thanks for your answer. I used shellcheck and got:

Code: Select all

In line 94:
   echo -e "\e[1;31m Ningún archivo de subtitulos en Seagate!\e[0m"
            ^-- SC1117: Backslash is literal in "\e". Prefer explicit escaping: "\\e".
                                                             ^-- SC1117: Backslash is literal in "\e". Prefer explicit escaping: "\\e".

and

Code: Select all

if ( cd $Seagate && ls *.torrent &> /dev/null ); then   
                       ^-- SC2035: Use ./*glob* or -- *glob* so names with dashes won't become options.

That's from the terminal when I run

Code: Select all

shellcheck ./my_script.sh

and from here https://www.shellcheck.net/ I got this

Code: Select all

Line 10:
if ( cd $Seagate && ls *.torrent &> /dev/null ); then  
                       ^-- SC2035: Use ./*glob* or -- *glob* so names with dashes won't become options.

but not the first one I got when running it locally on my system, why is that? BTW, I'm not sure but I think shellcheck got it wrong cause here https://github.com/koalaman/shellcheck/wiki/SC2035 says

Since files and arguments are strings passed the same way, programs can't properly determine which is which, and rely on dashes to determine what's what.

but if I change it to

Code: Select all

if ( cd $Seagate && ls ./*.torrent &> /dev/null ); then

apart form the the fact that now this "./" appears in front of every file name, I get the same result, meaning it works with no errors reported so all I do is some extra typing.

I read this https://mywiki.wooledge.org/ParsingLs too

EDIT: Thanks for mentioning shellcheck, it reminded me there was/is a new version on buster-backports, currently 0.7.1-1~bpo10+1, I installed that and now when run

Code: Select all

shellcheck ./my_script.sh

(actually I called it Xlist.sh) I got the same error as from the web version and the error old shellcheck's version, 0.5.0-3 no longer appears. So, thanks.

[Moltke]

I made some changes to the original script, it's not much but think it's an improvement from the previous version nonetheless

Code: Select all

#!/bin/bash
#listar archivos en sd card
#Creador: Moltke
#Octubre 2019
#El proposito principal es aprender a usar condicionales en un script.
#Algunas variables que hacen el script portable y reusable
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 

As you can see, just removed all the extra lines with cd $Seagate ... which did nothing but add even more redundancy to the code, it's not much but looks a bit better and works

[Head_on_a_Stick]

Trilby had some good suggestions over at the Arch forums: https://bbs.archlinux.org/viewtopic.php ... 2#p1924532

[Moltke]

Trilby had some good suggestions over at the Arch forums: https://bbs.archlinux.org/viewtopic.php ... 2#p1924532

Yeah, it did look promising but doesn't work as expected. This does though:

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

I didn't come with it 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 here https://unix.stackexchange.com/question ... do-instead are talking about.