13
\$\begingroup\$

I really like flac files, but my phone can't store them all. So I made a script for converting them, that slowly became a full featured flac organizer. I'd like to improve my programming skills, as well as the commenting. How can I improve?

The program is made out of 3 scripts:

config.sh

#!/bin/bash

#Script made for storing the users preference

rm -f config.txt


#FFmpeg don't come with an alias, sometimes
echo "If ffmpeg in your system has an alias, skip this passage"
echo "Do you wat to use the 'ffmpeg' alias?"
read YN
if [ "$YN" == "n" ]; then
echo "Standard ffmpeg directory is ~/bin"
DIR_FFMPEG="$HOME""/bin"
echo "Change it? (y = si, * = no)"
read YN
if [ "$YN" == "y" ]; then
    YN='';
    while [ "$YN" != "y" ]; do
    echo "Insert chosen directory"
    read -e DIR_FFMPEG

    if [ "$DIR_FFMPEG" == "yolo" ]; then #My ffmpeg folder is different, and I don't want to re-type everytime the folderm while debugging
        DIR_FFMPEG="$HOME""/Programmi/Compilati/ffmpeg/bin"
    fi

    echo "You chose the folder: $DIR_FFMPEG"
    ls "$DIR_FFMPEG"
    echo "Is it ok? (y = si, * = no)"
    read YN

    done
fi
    DIR_FFMPEG="$DIR_FFMPEG"/
fi

DIR_OUTPUT=""$HOME"/Musica"
echo "Standard music directory is ~/Musica"
echo "Modify it? (y = si, * = no)"
read YN

if [ "$YN" == "y" ]; then
    YN='';
    while [ "$YN" != "y" ]; do
        echo "Insert chosen directory"
        read -e DIR_OUTPUT
    if [ "$DIR_OUTPUT" == "yolo" ]; then #Same as ffmpeg
        DIR_OUTPUT="/media/nas"
    fi
        echo "You choose the directory: $DIR_OUTPUT"
    ls "$DIR_OUTPUT"
    echo "Is it ok? (y = si, * = no)"
        read YN
    done
    echo "DIR_OUTPUT" $DIR_OUTPUT >> log.txt
fi

echo "Select encoder and destination format"
echo
echo "  (a) libopus (e) libmp3lame  (i) vorbis  " #Tab for showing what encoder are possible
echo "  (b) libvorbis   (f) libfaac (l) mp2     "
echo "  (c) libfdk_aac  (g) eac3/ac3    (m) wmav2/wmav1 "
echo "  (d) aac     (h) libtwolame          "
echo
PROFILE=''
read ENCODER
case $ENCODER in #Just reciclyng a variable
    a)  ENCODER='libopus';
        EXTENSION='opus';;
    b)  ENCODER='libvorbis';
        EXTENSION='ogg';;
    c)  ENCODER='libfdk_aac'; #Damn you, fdk_aac
        echo "What profile do you want to use?"
        echo
        echo "  (*) Standard    (1) aac_he  (2) aac_he_v2   "
        read PROFILE
        case $PROFILE in
            1) PROFILE="-profile:a aac_he";;
            2) PROFILE="-profile:a aac_he_v2";;
            *) PROFILE="";;
        esac;
        EXTENSION='m4a';;
    d)  ENCODER='aac';
        EXTENSION='m4a';;
    e)  ENCODER='libmp3lame';
        EXTENSION='mp3';;
    f)  ENCODER='libfaac';
        EXTENSION='m4a';;
    g)  ENCODER='eac3/ac3';
        EXTENSION='ac3';;
    h)  ENCODER='libtwolame';
        EXTENSION='mp2';;
    i)  ENCODER='vorbis';
        EXTENSION='ogg';;
    l)  ENCODER='mp2';
        EXTENSION='mp2';;
    m)  ENCODER='wmav2/wmav1';
        EXTENSION='wmv';;
esac
echo
echo
echo "Select bitrate (Write only the number)"
echo
echo "  16Kb/s  64Kb/s  160Kb/s " #You can write every number, but those tabs are cool
echo "  32Kb/s  96Kb/s  192Kb/s "
echo "  48Kb/s  128Kb/s 320Kb/s "
echo
read BITRATE


echo "File di configurazione per lo script, modificare i valori o eseguire lo script 'config.sh'" >> config.txt
echo "FFMPEG =" $DIR_FFMPEG >> config.txt
echo "OUTPUT =" $DIR_OUTPUT >> config.txt
echo "ENCODER =" $ENCODER >> config.txt
echo "EXTENSION =" $EXTENSION >> config.txt
echo "PROFILE =" $PROFILE >> config.txt
echo "BITRATE =" $BITRATE >> config.txt

main.sh

#!/bin/bash

echo "Script for organizing Music Files"
echo
echo "Now the configuration file is this one" #The config should remain the same every time 
cat config.txt | sed 1d
echo
echo
echo "Modify it? (y=yes, *=no)"
read YN
if [ "$YN" == "y" ]; then
    YN=''
    ./config.sh
fi

#Load config file
CONFIG_FILE=$(cat config.txt | tr -d [:blank:])
FFMPEG=$(echo "$CONFIG_FILE"| grep FFMPEG | sed "s/FFMPEG=//")
DIR_OUTPUT=$(echo "$CONFIG_FILE"| grep OUTPUT | sed "s/OUTPUT=//")
ENCODER=$(echo "$CONFIG_FILE"| grep ENCODER | sed "s/ENCODER=//")
EXTENSION=$(echo "$CONFIG_FILE"| grep EXTENSION | sed "s/EXTENSION=//")
BITRATE=$(echo "$CONFIG_FILE"| grep BITRATE | sed "s/BITRATE=//")

#Input files
while [ "$YN" != "y" ]; do
    echo "Where are the music files to be organized?"
    read -e DIR_INPUT
    echo "You choose the folder: ""$DIR_INPUT" #Check
    ls "$DIR_INPUT" #Double check
    DIR_INPUT="$DIR_INPUT""/" 
    echo "Is it ok? (y = yes, * = no)"
    read YN
done

#Splitting CUE
CUE=0 #See later
if [ -e "$DIR_INPUT"*.cue ]; then
    echo "There is a .cue file"
    echo "Do you need to split the .flac? (y=yes, *=no)"
    read YN
    if [ "$YN" == "y" ]; then
    YN=''
        ./splitter_cue.sh "$DIR_INPUT"
    CUE=1 #Needed a condition variable
    DIR_INPUT_ORIGINAL="$DIR_INPUT"  #Needed for copying the flac/cue version of the file
    DIR_INPUT="$DIR_INPUT"/split/ #New folder with the splitted flac files
    fi
fi

#Load Metadata
rm metadata.txt
exiftool "$DIR_INPUT"*.flac >> metadata.txt
ARTISTA=$(grep -m 1 Artist metadata.txt)
PREFISSO="Artist                          : " #This variable make the whole thing cleaner, I guess
ARTISTA=$(echo "$ARTISTA" | sed "s/^$PREFISSO//")
ALBUM=$(grep -m 1 Album metadata.txt)
PREFISSO="Album                           : " #Same
ALBUM=$(echo "$ALBUM" | sed "s/^$PREFISSO//")
echo "The Artist is" $ARTISTA #Check for errors
echo "The Album is" $ALBUM
echo "Correct? (* = yes, n = no)"
read YN
if [ $YN == "n" ]; then #There Might be Errors
    echo "What is the Artist?"
    read ARTISTA
    echo "What is the Album?"
    read ALBUM
fi

#Kind of useless stuff
id3 -a "$ARTISTA" -l "$ALBUM" "$DIR_INPUT"*.flac #Id3 Tagging
id3 -2 -a "$ARTISTA" -l "$ALBUM" "$DIR_INPUT"*.flac #Id3v2 Tagging

#Folder Creation
DIR_OUTPUT=$(echo "$DIR_OUTPUT""/""$ARTISTA""/""$ALBUM")
if [ $CUE == 1 ]; then  #That condition... wait, I can do it in another way
    mkdir -p "$DIR_OUTPUT"/FLAC\ -\ CUE #This should be on the next part
    cp "$DIR_INPUT_ORIGINAL"/*.flac "$DIR_OUTPUT"/FLAC\ -\ CUE
    cp "$DIR_INPUT_ORIGINAL"/*.cue "$DIR_OUTPUT"/FLAC\ -\ CUE
fi
DIR_OUTPUT_FLAC=$(echo "$DIR_OUTPUT"/FLAC) #I really Hate This Variable
DIR_OUTPUT=$(echo "$DIR_OUTPUT""/"$(echo "$EXTENSION" | tr [:lower:] [:upper:])" - ""$BITRATE"" KBs") # I need the extension to be uppercase, or I'll freak out
echo "$DIR_OUTPUT" #Debug

#ACTUAL Folder Creation
mkdir -p "$DIR_OUTPUT"/
mkdir -p "$DIR_OUTPUT_FLAC"
echo "DIR_OUTPUT" $DIR_OUTPUT >> log.txt #Logs are always useful

#Converter
echo "Thanks to rubylaser for making the scheletron of this"
echo "http://ubuntuforums.org/showthread.php?t=1705974"
for i in "$DIR_INPUT"*.flac; do
    if [ -e "$i" ]; then
    file=$(basename -s .flac "$i") #.flac.opus isn't cool
    "$DIR_FFMPEG"ffmpeg -i  "$i" -c:a "$ENCODER" -b:a "$BITRATE"k  "$DIR_OUTPUT"/"$file"."$EXTENSION"
    cp "$i" "$DIR_OUTPUT_FLAC" #Don't fotget the actual flac file
    fi
done

#Cover Copier
#To be done
echo
echo
echo "You'll need to copy folder and .accurip files by your own"
echo "Sorry :D"

splitter_cue.sh

#!/bin/sh

# frontend for:            cuetools, shntool, mp3splt
# optional dependencies:    flac, mac, wavpack, ttaenc
# v1.3 sen

echo "Thanks to sen for making this script"
echo "https://bbs.archlinux.org/viewtopic.php?id=75774"
sleep 1s #He deserve it

SDIR=`pwd`

if [ "$1" = "" ]
  then
    DIR=$SDIR
else
    case $1 in
        -h | --help )
            echo "Usage: cuesplit [Path]"
            echo "       The default path is the current directory."
            exit
            ;;
        * )
        DIR=$1
    esac
fi

echo -e "\

Directory: $DIR
________________________________________
"
cd "$DIR"
TYPE=`ls -t1`

case $TYPE in
    *.ape*)
        mkdir split
        shnsplit -d split -f *.cue -o "flac flac -V --best -o %f -" *.ape -t "%n - %t"
        rm -f split/00*pregap*
        cuetag.sh *.cue split/*.flac
        exit
        ;;

    *.flac*)
        mkdir split
        shnsplit -d split -f *.cue -o "flac flac -V --best -o %f -" *.flac -t "%n - %t"
        rm -f split/00*pregap*
        cuetag.sh *.cue split/*.flac
        exit
        ;;

    *.mp3*)
        mp3splt -no "@n @p - @t (split)" -c *.cue *.mp3
        cuetag.sh *.cue *split\).mp3
        exit
        ;;

    *.ogg*)
        mp3splt -no "@n @p - @t (split)" -c *.cue *.ogg
        cuetag.sh *.cue *split\).ogg
        exit
        ;;

    *.tta*)
        mkdir split
        shnsplit -d split -f *.cue -o "flac flac -V --best -o %f -" *.tta -t "%n - %t"
        rm -f split/00*pregap*
        cuetag.sh *.cue split/*.flac
        exit
        ;;

    *.wv*)
        mkdir split
        shnsplit -d split -f *.cue -o "flac flac -V --best -o %f -" *.wv -t "%n - %t"
        rm -f split/00*pregap*
        cuetag.sh *.cue split/*.flac
        exit
        ;;

    *.wav*)
        mkdir split
        shnsplit -d split -f *.cue -o "flac flac -V --best -o %f -" *.wav -t "%n - %t"
        rm -f split/00*pregap*
        cuetag.sh *.cue split/*.flac
        exit
        ;;

    * )
    echo "Error: Found no files to split!"
    echo "       --> APE, FLAC, MP3, OGG, TTA, WV, WAV"
esac
exit
\$\endgroup\$
1
\$\begingroup\$

config.sh is OK, as far as it goes. Some people prefer the (composable) getopts style of collecting arguments from command line -- it appears you prefer a chatty style of repeated prompting, that's fine.

There are some repeated code snippets, so you would benefit from defining a helper function:

yorn() {
    read YN
    test $YN = y
}

If you like, a while loop could ensure that only valid 'y' or 'n' inputs are accepted.

In several places you use odd quoting, such as DIR_FFMPEG="$HOME""/bin". Better to elide the extra quotes around /bin. Also, you either intend to support embedded blanks in $HOME, or you do not. If not, better to elide all quotes. If you support blanks, then you'll need to carefully quote derived variables like $DIR_FFMPEG further down in the script. Your quoting was inconsistent, so I had a hard time telling whether you wanted to support embedded blanks (for directory names like "Program Files").

Similar remarks for that pattern in main.sh, probably better to simply say, e.g.:

DIR_OUTPUT=$DIR_OUTPUT/$ARTISTA/$ALBUM

You correctly spelled FLAC\ -\ CUE, but it might be more naturally expressed as "FLAC - CUE". If you have the flexibility to rename it to FLAC-CUE you might find that simpler -- fewer quoting hassles.

It is probably more natural to not append a slash with DIR_INPUT="$DIR_INPUT""/", and to improve readability by tacking on the slash with each $DIR_INPUT reference, e.g. $DIR_INPUT/*.flac.

The if [ -e "$i" ] test seems redundant.

The usual idiom for the first if in splitter_cue.sh looks like this:

DIR = ${1:-`pwd`}

It allows for simple defaulting.

\$\endgroup\$
1
\$\begingroup\$

Consider storing the preferences as shell commands

The config.txt file is very nearly in shell syntax. If you write to it like this:

{
    echo '# File di configurazione per lo script, modificare i valori o eseguire lo script '"'config.sh'"
    for i in DIR_FFMPEG DIR_OUTPUT ENCODER EXTENSION PROFILE BITRATE
    do printf '%s=%q\n' $i ${!i}
    done
} > config.txt

then it's trivial to read the values back:

. config.txt

If you do this, do consider checking first that the file and its parent directories aren't writeable by other users, as that would make it untrusted input (you already have that problem, as $DIR_FFMPEG/ffmpeg is executed without any checking).

Read config in config.sh

It's irritating to have to re-enter all values when you want to change just one, so if the config file pre-exists, read it first to establish some default choices

Use read -p to prompt

read -p "Do you want to use the 'ffmpeg' alias? [Y/n] " YN

Actually, I'd re-write the whole of that to fall back to finding ffmpeg in $PATH:

# unset if not executable
test -x "${FFMPEG:=`which ffmpeg`}" || FFMPEG=

until read -p "Location of ffmpeg: ${FFMPEG:+(default $FFMPEG) }: " && test -x "${REPLY:=$FFMPEG}"
do printf '%q: not executable\n' "$REPLY" >&2
done
FFMPEG="$REPLY"

Use select to choose from a list

Instead of hand-coding the logic to choose from a list, let Bash do that for you with a select compound command.


Suggested new config.sh

#!/bin/bash
config_file=config.txt

if test -r "$config_file"
then . "$config_file"
fi

test -x "${FFMPEG:=`which ffmpeg`}" || FFMPEG=
until read -e -p "Location of ffmpeg: " -i "$FFMPEG" \
        && test -x "$REPLY"
do printf '%q: not executable\n' "$REPLY" >&2
done
FFMPEG="$REPLY"

test -d "${DIR_OUTPUT:=$HOME/Musica}" || DIR_OUTPUT=
until read -e -p "Output directory: " -i "$DIR_OUTPUT" \
        && test -d "$REPLY" \
        && test -w "$REPLY"
do printf '%q: not a writeable directory\n' "$REPLY" >&2
done
DIR_OUTPUT="$REPLY"

echo "Select encoder and destination format" >&2
select ENCODER in \
    libopus libvorbis libfdk_aac aac libmp3lame \
            libfaac eac3/ac3 libtwolame vorbis mp2 wmav2/wmav1
do
    case $ENCODER in
        libfdk_aac)
            EXTENSION='m4a';
            echo "Which profile do you want to use?" >&2
            select PROFILE in '(default)' aac_he aac_he_v2
            do
                case $PROFILE in
                    aac_*) PROFILE="-profile:a $PROFILE" break;;
                    *) PROFILE='' break;;
                esac
            done
            ;;
        libopus) EXTENSION='opus';;
        libvorbis) EXTENSION='ogg';;
        aac) EXTENSION='m4a';;
        libmp3lame) EXTENSION='mp3';;
        libfaac) EXTENSION='m4a';;
        eac3/ac3) EXTENSION='ac3';;
        libtwolame) EXTENSION='mp2';;
        vorbis) EXTENSION='ogg';;
        mp2) EXTENSION='mp2';;
        wmav2/wmav1) EXTENSION='wmv';;
        *) continue;; # not recognised
    esac
    break
done
read -e -p "Select bit rate in kb/s: [16/32/48/64/96/128/160/192/320/...] " -i "$BITRATE" BITRATE

{
    echo '# File di configurazione per lo script'
    echo "# modificare i valori o eseguire lo script 'config.sh'"
    for i in FFMPEG DIR_OUTPUT ENCODER EXTENSION PROFILE BITRATE
    do printf '%s=%q\n' $i "${!i}"
    done
} >"$config_file"

exit

I hope to be able to review the other scripts soon.

\$\endgroup\$

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.