Can you give some ideas on how to improve script?

[Tolkem]

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: Tolkem
#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/tolkem/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: Tolkem = Creator: Tolkem
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.

 

[JasKinasis]

The first thing I notice is that you have a lot of duplicate code.
And wherever you have duplicate code, that is almost always a good place to put a function.

What you're doing in each block of code is essentially exactly the same thing every time. You're checking the seagate drive for files of a particular type.
If any files were found, you run the ls command a second time to display the files. Otherwise you show a message saying that there are none.
The only difference is the file-pattern each time - e.g. "*.torrent", or "*.png" and the file-type in the messages regarding files being found, or not found.

Also, running the ls command twice is a little redundant too. You could run ls once and store any results in a variable. Then it's just a case of checking whether or not the variable is empty and displaying an appropriate message. And display the content of the variable, if it is not empty.

Also there's no need to cd into the directory either.

But all of that aside - you could put that repeated bit of code into a function and then at runtime - when the function is called, you could pass it the two things that ARE different each time.
e.g. the file-pattern and the name for the file-type.

But then you're going to have a lot of calls to the function in your code - so again, there will still be some repetition in the code. So is there a way we can make this even more efficient.....

In this case, YES!

Rather than creating a function - personally I'd put a pipe-and space-delimited "| " list containing pairs of file-patterns and names into a heredoc and then process that inside a while loop.

Everything I've talked about so far will look something like this:
(NOTE: I'll explain the code afterwards)

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

Now you have a very small, succinct script where the only thing that is repeating itself is a loop that processes the content of the heredoc at the end of the script.

On each iteration of the while loop we do the following:
1. Read a line containing a pair of values into an array.
NOTE: The first item in the array (item 0) is the search pattern, the second (item 1) is the name for the file-type.
2. Display $linea_blanca
3. Use ls to search $seagate for files with names containing the search pattern (${arr[0]}) and store any results in a variable called foundFiles.
4. If foundFiles is empty
- display the "Ningún archivo" message, where ${arr[1]} is the name for the file-type.
5. Otherwise we:
- Display the "Archivos" message, again where ${arr[1]} is the name for the file-type.
- And display the list of file-names stored in foundFiles

Note - because we didn't cd into the $seagate directory, I've piped the file-names through basename via xargs, in order to filter out the path to the seagate drive, so you're just left with the file-names.

And then the loop continues like that, reading and processing each search-pattern/name pair until we reach the end of the heredoc. Which is delimited by EOM.

If you want to add another file-type to search for - you can simply add a line of text to the heredoc containing the file-pattern and the name for that file-type.

I hope this has been somewhat useful!

[edit]
One final thing for you to take note of is:
The script is only looking in the root of your seagate drive - it's not going to look in any sub-directories.
I assumed that this was a deliberate decision on your part.
If you do want to search recursively - the script will need some additional modification.
[/edit]

 

[Tolkem]

@JasKinasis thank you so much! I can't almost believe that script could look like that! With your modifications it looks awesome!

The first thing I notice is that you have a lot of duplicate code ... Also, running the ls command twice is a little redundant too. You could run ls once and store any results in a variable. Then it's just a case of checking whether or not the variable is empty and displaying an appropriate message. And display the content of the variable, if it is not empty.

Yes, I was/am well aware of the redundancy on the code, though I wanted to practice if statements in a script and it served well for that purpose, later I wanted to improve that but I like said, I just couldn't figure it out by myself. I did try to use some arrays but nothing near what you did there lol I was doing something like

files=(torrent mp3 zip srt txt png m?? pdf sh iso aria2)

and tried to call that but always failed and now I see why it did. I tried different versions of that by reading here https://tldp.org/LDP/Bash-Beginners-Guide/html/sect_10_02.html and other sites with no success, again, now I see why.

One final thing for you to take note of is:
The script is only looking in the root of your seagate drive - it's not going to look in any sub-directories.
I assumed that this was a deliberate decision on your part.
If you do want to search recursively - the script will need some additional modification.

Yes, I only wanted it to look for files in the root of the drive, not recursively, would you mind telling me how to do that as well? I'd really appreciate your help man, thank you very much again. Can you point me to a good tutorial on bash/shell scripting? I've read a few as well as some books like the command line but always got stuck at some point, is like I hit a wall I can past through. Same thing with python. I read books, posts, blogs ... watched videos ... I'm registered (free membership) on codeacademy too but don't know, always hit that wall and can't find a way to go over it. Again, thanks for taking the time doing this. Truly, greatly, very much appreciated your help!

 

[Tolkem]

Hmmm ... I copied/pasted your code to a file, saved it and ran it, doesn't work; no files are listed though they exist on the drive. Also, I ran shellcheck against it and got this

In  line 7:
    foundFiles="$(ls $seagate/${arr[0]} 2> /dev/null)"
^-------^ SC2086: Double quote to prevent globbing and word splitting.

Did you mean:
foundFiles="$(ls $seagate/"${arr[0]}" 2> /dev/null)"

For more information:
https://www.shellcheck.net/wiki/SC2086 -- Double quote to prevent globbing ..

and don't understand the "did you mean" line since when I look at it, it's already like that. Am I missing something there?

EDIT: Found what shellcheck was complaining about, still, doesn't work; all I get is "No files with .ext in Seagate"

 

[JasKinasis]

Ooops! Sorry - I just noticed in the code I posted in my post IFS was set to:

IFS = ", "

But it should have been:

IFS="| "

I've updated the code in my previous post to correct the issue!

Now that IFS is set correctly, it should work. Sorry about that!

 

[Tolkem]

Ooops! Sorry - I just noticed in the code I posted in my post IFS was set to:

IFS = ", "

But it should have been:

IFS="| "

I've updated the code in my previous post to correct the issue!

Now that IFS is set correctly, it should work. Sorry about that!

I edited the code as you mentioned but still doesn't work as expected; no files are listed even though they're there. I played a bit with it yesterday and changed that line too, except I moved IFS="| " at the top, so it looked like

#!/usr/bin/env bash
seagate="/media/moltke/Seagate"
linea_blanca="\e[1;37m ++++++++++++++++++++++++\e[0m"
IFS=" | "
while  read -ra arr; do
    echo -e "$linea_blanca"
    foundFiles="$(ls $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 saw that format in a book, linux shellscript cookbook, while searching for a fix but same result. I'll keep reading and playing a bit with it to see if can fix it. Thanks for your help, much appreciated.
BTW, sorry for the late reply but my internet went down yesterday and came back just an hour ago or so.

Also, I edited the original script and while it's still nowhere near what you did, think it's nonetheless an improvement from the previous version:

#!/bin/bash
#listar archivos en sd card
#Creador: Tolkem
#Octubre 2019
#El proposito principal es aprender a usar condicionales en un script.
#Algunas variables que hacen el script portable y reusable
Seagate="/media/tolkem/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 lines containing cd $Seagate ... as per your suggestion

Also there's no need to cd into the directory either.

it's not much but looks a bit better and works

 

[JasKinasis]

I edited the code as you mentioned but still doesn't work as expected; no files are listed even though they're there. I played a bit with it yesterday and changed that line too, except I moved IFS="| " at the top, so it looked like

IFS=" | "

The reason it's not working is becuase you used:
IFS=" | "

It's supposed to be:
IFS="| "

You added an extra space!

The script I've posted definitely works, because I tested it on my laptop last night, after I corrected the previous error!

 

[Tolkem]

The reason it's not working is becuase you used:
IFS=" | "

Sorry, I didn't explain myself clear enough. What I meant by that is that while I played with it yesterday I did that and it didn't work but

It's supposed to be:
IFS="| "

You added an extra space!

I did it just like you said and still get; No files with .ext in Seagate.

The script I've posted definitely works, because I tested it on my laptop last night, after I corrected the previous error!

Well, it doesn't for me as expected since files are not being listed. Take a look:

But the previous version does list the files

Also, I kind of remember you mentioning you use Debian testing while I'm doing this in Debian stable, maybe bash different versions have something to with it? I have a Bullseye VM so I might try and see whether or not that might be the case. Thanks for the help.

 

[JasKinasis]

Hmm, strange!
I'll take another look at this when I get home from work later!
It definitely works for me! :/

I was running Debian testing. But when Debian 10 was released, I switched to the stable repo. I usually do that when a new version of stable is released. Sometimes the testing repo is a bit twitchy for a month or so after a new release.

I usually switch back to testing after a couple of months. This time, I just haven't gotten around to switching back to testing yet!

But I'm very happy with the software included in Debian 10 though, so I'm half-tempted to keep tracking stable.....

 

[Tolkem]

Hmm, strange!
I'll take another look at this when I get home from work later!
It definitely works for me! :/

I was running Debian testing. But when Debian 10 was released, I switched to the stable repo. I usually do that when a new version of stable is released. Sometimes the testing repo is a bit twitchy for a month or so after a new release.

I usually switch back to testing after a couple of months. This time, I just haven't gotten around to switching back to testing yet!

But I'm very happy with the software included in Debian 10 though, so I'm half-tempted to keep tracking stable.....

I found where the issue was, in your original edit of my script you did

while IFS="| " read -ra arr; do
echo -e "$linea_blanca"
foundFiles="$(ls $seagate/${arr[0]} 2> /dev/null)" ---> it had to be "$seagate"
if [[ -z "$foundFiles" ]] ; then
echo -e "\e[1;31m Ningún archivo ${arr[1]} en seagate:\e[0m"

I changed that to this

while IFS="| " read -ra arr; do
    echo -e "$linea_blanca"
    foundFiles="$(ls "$seagate"/${arr[0]} 2> /dev/null)"
    if [[ -z "$foundFiles" ]] ; then
        echo -e "\e[1;31m Ningún archivo ${arr[1]} en seagate:\e[0m"

and now it works However, I have to deal with spaces in some files' names so the output's a bit cleaner, though I'm not sure on how to ... yet. On a side note, think I now understand a little bit more what people here are talking about. Thanks again! I've learnt a lot much more with this excercise than I have by reading articles, posts, books and watching videos! Very much appreciated your help!