[SOLVED] I need help making this bash code cleaner

GamezR2EZ · 02-13-2012, 03:19 PM

This is a project I am working on just to better understand some concepts. It has the added advantage of begin semi useful to me.

Alright I have a list of songs I need to parse. The list is from freedb and looks like the following:

Code:

DISCID=abcdef01
DTITLE=Artist / Album
DYEAR=2006
DGENRE=Classical
TTITLE0=Song 1
TTITLE1=Song 2
TTITLE2=Song 3
TTITLE3=Song 4
TTITLE4=Song 5
TTITLE5=Song 6
TTITLE6=Song 7
TTITLE7=Song 8
TTITLE8=Song 9
TTITLE9=Song 10
TTITLE10=Song 11
TTITLE11=Song 12
TTITLE12=Song 13
TTITLE13=Song 14
EXTD= YEAR: 2006 ID3G: 47
EXTT0=
EXTT1=
EXTT2=
EXTT3=
EXTT4=
EXTT5=
EXTT6=
EXTT7=
EXTT8=
EXTT9=
EXTT10=
EXTT11=
EXTT12=
EXTT13=
PLAYORDER=
.

I need to parse that so I have Artist, Album, Year, and song names. Here is what I have so far:

Code:

while read line
do
if echo $line | grep DTITLE > /dev/null
  then ARTIST=$(echo "$line" | sed -e 's/DTITLE=//' | sed -e 's/ \/.*//')
       ALBUM=$(echo "$line" | sed -e 's/.*\/ //')
elif echo $line | grep DYEAR > /dev/null
  then YEAR=$(echo "$line" | sed -e 's/DYEAR=//')
elif echo $line | grep TTITLE0= > /dev/null
  then TRACK01=$(echo "$line" | sed -e 's/TTITLE0=//')
elif echo $line | grep TTITLE1= > /dev/null
  then TRACK02=$(echo "$line" | sed -e 's/TTITLE1=//')
elif echo $line | grep TTITLE2= > /dev/null
  then TRACK03=$(echo "$line" | sed -e 's/TTITLE2=//')
elif echo $line | grep TTITLE3= > /dev/null
  then TRACK04=$(echo "$line" | sed -e 's/TTITLE3=//')
elif echo $line | grep TTITLE4= > /dev/null
  then TRACK05=$(echo "$line" | sed -e 's/TTITLE4=//')
elif echo $line | grep TTITLE5= > /dev/null
  then TRACK06=$(echo "$line" | sed -e 's/TTITLE5=//')
elif echo $line | grep TTITLE6= > /dev/null
  then TRACK07=$(echo "$line" | sed -e 's/TTITLE6=//')
elif echo $line | grep TTITLE7= > /dev/null
  then TRACK08=$(echo "$line" | sed -e 's/TTITLE7=//')
elif echo $line | grep TTITLE8= > /dev/null
  then TRACK09=$(echo "$line" | sed -e 's/TTITLE8=//')
elif echo $line | grep TTITLE9= > /dev/null
  then TRACK10=$(echo "$line" | sed -e 's/TTITLE9=//')
elif echo $line | grep TTITLE10= > /dev/null
  then TRACK11=$(echo "$line" | sed -e 's/TTITLE10=//')
elif echo $line | grep TTITLE11= > /dev/null
  then TRACK12=$(echo "$line" | sed -e 's/TTITLE11=//')
elif echo $line | grep TTITLE12= > /dev/null
  then TRACK13=$(echo "$line" | sed -e 's/TTITLE12=//')
elif echo $line | grep TTITLE13= > /dev/null
  then TRACK14=$(echo "$line" | sed -e 's/TTITLE13=//')
elif echo $line | grep TTITLE14= > /dev/null
  then TRACK15=$(echo "$line" | sed -e 's/TTITLE14=//')
elif echo $line | grep TTITLE15= > /dev/null
  then TRACK16=$(echo "$line" | sed -e 's/TTITLE15=//')
elif echo $line | grep TTITLE16= > /dev/null
  then TRACK17=$(echo "$line" | sed -e 's/TTITLE16=//')
elif echo $line | grep TTITLE17= > /dev/null
  then TRACK18=$(echo "$line" | sed -e 's/TTITLE17=//')
elif echo $line | grep TTITLE18= > /dev/null
  then TRACK19=$(echo "$line" | sed -e 's/TTITLE18=//')
elif echo $line | grep TTITLE19= > /dev/null
  then TRACK20=$(echo "$line" | sed -e 's/TTITLE19=//')
elif echo $line | grep TTITLE20= > /dev/null
  then TRACK21=$(echo "$line" | sed -e 's/TTITLE20=//')
elif echo $line | grep TTITLE21= > /dev/null
  then TRACK22=$(echo "$line" | sed -e 's/TTITLE21=//')
fi
done < <(echo "$freedbalbuminfo")

Not eloquent at all, I know. That is where I need some help. It is fully functional, but not dynamic.

The variables are as follows:

Code:

$freedbalbuminfo contains the freedb response with all of the information in the first code block

$numtracks contains the number of tracks on the disc

I need a dynamic way of parsing/declaring the TRACK names without the above kludge.

I am willing to rewrite the entire section, if there is a better way of doing this.

Please note:
The above code works! It is just not clean.
This is a learning experience for me, I am comfortable with bash, grep, sed, ect, but I am no guru.

---Solved---

I ended up using arrays as millgates suggested. The final code is below.

Code:

tracknames=()
while read LINE
do
  case "$LINE" in
	DTITLE*)
		ARTIST=$(echo "$LINE" | sed -e 's/.*=//' -e 's/ \/ .*//')
		ALBUM=$(echo "$LINE" | sed -e 's/.* \/ //')
	;;
	DYEAR*)
		YEAR=$(echo "$LINE" | sed -e 's/.*=//')
	;;
	TTITLE*)
		TRACKNAMES+=("$(echo "$LINE" | sed -e 's/.*=//')")
	;;
  esac
done < <(echo "$FREEDBREAD")

millgates · 02-13-2012, 04:21 PM

How about using arrays? It could look like this:

Code:

#!/bin/bash

declare -a TRACKS
declare -a TNUMS

while read line; do
	if echo $line | grep DTITLE > /dev/null; then
	       	ARTIST=$(echo "$line" | sed -e 's/DTITLE=//' | sed -e 's/ \/.*//')
		ALBUM=$(echo "$line" | sed -e 's/.*\/ //')
	elif echo $line | grep DYEAR > /dev/null; then
	       	YEAR=$(echo "$line" | sed -e 's/DYEAR=//')

	# this handles all the titles	
	elif echo "$line" | grep TTITLE > /dev/null; then
		TNUMS[${#TNUMS[@]}]="$(echo "$line"|sed 's/TTITLE\([0-9]*\)=.*/\1/')"
		TRACKS[${#TRACKS[@]}]="$(echo "$line"|sed 's/TTITLE[0-9]*=\(.*\)/\1/')"
	fi
done < <(echo "$freedbalbuminfo")

# you can then print the numbers  and tracks like this:
for ((i=0; i<${#TRACKS[@]}; i++)); do
	echo "TRACK${TNUMS[$i]} -- \"${TRACKS[$i]}\""
done

Hope this helps

kbp · 02-13-2012, 04:31 PM

I can't see why you don't just source the file, it looks like valid bash syntax.

Code:

. <path_to_file>
ARTIST=$(echo $DTITLE | cut -d"/" -f1 | tr -d " ")
ALBUM=$(echo $DTITLE | cut -d"/" -f2 | tr -d " ")
YEAR=$DYEAR
TRACK01=$TTITLE0
...

millgates · 02-13-2012, 04:36 PM

Quote:

Originally Posted by kbp

I can't see why you don't just source the file, it looks like valid bash syntax.

Yes, but that will create a bunch of variables TRACK1, TRACK2, etc., which is inconvenient to work with. The OP would have to use indirect variables or the e-word (don't hit me!). It is more practical to use arrays, I guess.

Cedrik · 02-13-2012, 04:48 PM

Why not try pattern matching like

Code:

if [[ $line == DTITLE* ]]; then
  ...

if [[ $line == TTITLE* ]]; then
  ...

Quote:

Originally Posted by millgates

the e-word (don't hit me!)

Beware of the val police

GamezR2EZ · 02-13-2012, 06:13 PM

Quote:

Originally Posted by kbp

I can't see why you don't just source the file, it looks like valid bash syntax.

I would, but one of the things I was trying to learn more about is parsing. But you are right, it is bash syntax.

millgates, arrays of course!! Why didn't I think of that?

Thank you for the code, I wrote some from scratch though (learning experience...). I just appended the string to the array. Also I used sed to chop off everything from = back (which is what I should have done before anyway....).

Thanks guys!! If you think there is more I can do, suggestions are welcome. But I am happy right now.

The final code:

Code:

tracknames=()
while read line
do
if echo $line | grep DTITLE > /dev/null
  then ARTIST=$(echo "$line" | sed -e 's/.*=//' | sed -e 's/ \/.*//')
       ALBUM=$(echo "$line" | sed -e 's/.*\/ //')
elif echo $line | grep DYEAR > /dev/null
  then YEAR=$(echo "$line" | sed -e 's/DYEAR=//')
elif echo $line | grep TTITLE > /dev/null
  then tracknames+=("$(echo "$line" | sed -e 's/.*=//')")
fi
done < <(echo "$freedbread")

grail · 02-13-2012, 10:57 PM

Just in case you wanted to use all bash (ie no grep or sed):

Code:

tracks=()

while read -r line
do
	if [[ $line =~ DTITLE ]]
	then
		ALBUM=${line#* / }
		ARTIST=${line% /*}
	elif [[ $line =~ DYEAR ]]
	then
		YEAR=${line#*=}
	elif [[ $line =~ TTITLE ]]
	then
		tracks+=("${line#*=}")
	fi
done< <(echo "$freedbread")

gnashley · 02-14-2012, 12:04 AM

For pattern matching, a 'case' statement would be much faster and cleaner looking:

Code:

case $line in
  *DTITLE*) do something ;;
  *DYEAR*) do something ;;

esac

GamezR2EZ · 02-14-2012, 08:11 AM

Quote:

Originally Posted by gnashley

For pattern matching, a 'case' statement would be much faster and cleaner looking:

Thanks! I had already converted it to a case before your comment. And you are correct, it is much cleaner.

gnashley · 02-14-2012, 09:34 AM

It also is very much faster -especially if you are using grep instead of bash regexp.
I had a routine which used *lots* of if [[]]'s in a loop. The same functionality using case ran 30 times faster! If you think about it, using 'if [[ ..' calls two internal bash functions already -whether you use any fancy matching or not.