Skip to main content
added 1 character in body
Source Link
janos
  • 113.1k
  • 15
  • 154
  • 396

Putting it all togetherSuggested implementation

Putting it all together

Suggested implementation

Bounty Awarded with 100 reputation awarded by Sarima
added 1477 characters in body
Source Link
janos
  • 113.1k
  • 15
  • 154
  • 396

Looking at this code, there's still a lot of repetition: the processing of each image type looks very similar. You could go a step further and generalize that logic too, for example:

process_image() {
    TITLE=$1; shift
    IMAGE=$1; shift
    DIR=$1; shift
    CP_FUNCTION=$1; shift
    echo $TITLE
    if [ -s "$IMAGE" ] 
    then
        echo " -> Found new file to be used!"
        prepare_dir "$DIR"

        echo " -> Copying new files, please wait!"
        $CP_FUNCTION "$IMAGE" "$DIR"

        echo " -> Applying changes to project!"
        cp "$DIR"/* "$DSTDIR"
    else
        echo " -> Could not find new image to use, skipping!"
    fi
}

cp_main() {
    IMAGE=$1; shift
    DIR=$1; shift
    cp "$IMAGE" "$DIR"/Main_Page.png
    cp "$IMAGE" "$DIR"/Main_Page2.png
    # ... and so on ...
}

process_image "Main Background:" "$SRCDIR"/New_Main.png "$WORKDIR"/Main cp_main

cp_help() {
    IMAGE=$1; shift
    DIR=$1; shift
    cp "$IMAGE" "$DIR"/Help_Page.png
    cp "$IMAGE" "$DIR"/Help_Page2.png
    # ... and so on ...
}

process_image "Help Pages:" "$SRCDIR"/New_Help.png "$WORKDIR"/Help cp_help

... but this risks the script becoming too cryptic. At some point you have to draw the line and find the right balance between optimization and overengineering.

Looking at this code, there's still a lot of repetition: the processing of each image type looks very similar. You could go a step further and generalize that logic too, for example:

process_image() {
    TITLE=$1; shift
    IMAGE=$1; shift
    DIR=$1; shift
    CP_FUNCTION=$1; shift
    echo $TITLE
    if [ -s "$IMAGE" ] 
    then
        echo " -> Found new file to be used!"
        prepare_dir "$DIR"

        echo " -> Copying new files, please wait!"
        $CP_FUNCTION "$IMAGE" "$DIR"

        echo " -> Applying changes to project!"
        cp "$DIR"/* "$DSTDIR"
    else
        echo " -> Could not find new image to use, skipping!"
    fi
}

cp_main() {
    IMAGE=$1; shift
    DIR=$1; shift
    cp "$IMAGE" "$DIR"/Main_Page.png
    cp "$IMAGE" "$DIR"/Main_Page2.png
    # ... and so on ...
}

process_image "Main Background:" "$SRCDIR"/New_Main.png "$WORKDIR"/Main cp_main

cp_help() {
    IMAGE=$1; shift
    DIR=$1; shift
    cp "$IMAGE" "$DIR"/Help_Page.png
    cp "$IMAGE" "$DIR"/Help_Page2.png
    # ... and so on ...
}

process_image "Help Pages:" "$SRCDIR"/New_Help.png "$WORKDIR"/Help cp_help

... but this risks the script becoming too cryptic. At some point you have to draw the line and find the right balance between optimization and overengineering.

Source Link
janos
  • 113.1k
  • 15
  • 154
  • 396

Extract repeated values to variables

The most important thing is to avoid copy-pasting absolute paths everywhere. If the path ever changes it's a hassle to replace all strings in the script. It also makes it difficult to test the script with dummy directories instead of the real ones. So, always constants in variables, for example:

SRCDIR=/disk/usbsda1
WORKDIR=/opt/pclient/projekte
DSTDIR=/opt/pclient/projekte/default_prj/terminal_files

Similarly, when using these constants repeatedly, it's good to introduce a temporary variable to capture the common element. For example, instead of this:

cp "$SRCDIR"/New_Main.png "$WORKDIR"/Main/Main_Page.png
cp "$SRCDIR"/New_Main.png "$WORKDIR"/Main/Main_Page2.png
cp "$SRCDIR"/New_Main.png "$WORKDIR"/Main/Main_Page3.png

I would do like this:

IMAGE="$SRCDIR"/New_Main.png
DIR="$WORKDIR"/Main
cp "$IMAGE" "$DIR"/Main_Page.png
cp "$IMAGE" "$DIR"/Main_Page2.png
cp "$IMAGE" "$DIR"/Main_Page3.png

These kind of extractions to variables make the script more flexible, as you can change a path in one place and it will affect all the places where it's used. Another good thing is that a variable serves as a label of the purpose, the intention, which can be often more descriptive than the actual hardcoded string.

A note about quoting

I didn't quote SRCDIR=/disk/usbsda1 because it's unnecessary. But afterwards I quote "$SRCDIR" everywhere, in case "somebody" might ever set SRCDIR to a path with spaces. This is good precaution. Finally, these are equivalent:

cp "$IMAGE" "$DIR"/Main_Page.png
cp "$IMAGE" "$DIR/Main_Page.png"

Extract repeated logic to functions

In the processing each of the image types, you use the same kind of logic to check if a directory exists and remove its contents, or else create the directory. This could be in a function:

prepare_dir() {
    DIR=$1
    if [ -d "$DIR" ]
    then
        echo " -> Found old directory, removing contents!"
        rm -rf "$DIR"/*
    else
        echo " -> Creating new directory!"
        mkdir -p "$DIR"
    fi   
}

You can call this with prepare_dir "$WORKDIR"/Main, prepare_dir "$WORKDIR"/Help, prepare_dir "$WORKDIR"/Logo, and so on, reducing duplication and shortening your code.

Excessive comments

Your many echo statements already explain what the code does. The comments are redundant, I would remove all of them.

Moving code to the right enclosing block

In the processing of some of the image types, you run a cp after the main processing, even if a matching file did not exist, for example:

echo " -> Now creating Help logos"
if [ -s "/disk/usbsda1/New_Help.png" ] 
then
    echo " -> Found new file to be used!"
    # ...
    echo " -> Copying new files, please wait!"
    cp /disk/usbsda1/New_Help.png /opt/pclient/projekte/Help/Help_Page.png
    # ...
else
    echo " -> Could not find new image to use, skipping!"
fi

echo " -> Applying changes to project!"
cp /opt/pclient/projekte/Help/* /opt/pclient/projekte/default_prj/terminal_files/

The last cp is outside of the if-then block. But intuitively, it seems that if the if condition was false, then this last cp would find no files to copy, in which case it should be within the then block above.

Misc

Do you really need to copy files to a temporary directory first? Based on just the script you posted, this seems pointless, you could have copied directly to the destination.

Putting it all together

Applying the suggestions above, I would rewrite your script like this (but without spelling out all the individual files in some of the longer blocks):

#! /bin/sh

SRCDIR=/disk/usbsda1
WORKDIR=/opt/pclient/projekte
DSTDIR=/opt/pclient/projekte/default_prj/terminal_files

prepare_dir() {
    DIR=$1
    if [ -d "$DIR" ]
    then
        echo " -> Found old directory, removing contents!"
        rm -rf "$DIR"/*
    else
        echo " -> Creating new directory!"
        mkdir -p "$DIR"
    fi   
}

echo "
****Project Customisation Script STARTING****
"
echo "Main Background:"
IMAGE="$SRCDIR"/New_Main.png
if [ -s "$IMAGE" ] 
then
    echo " -> Found new file to be used!"
    DIR="$WORKDIR"/Main
    prepare_dir "$DIR"

    echo " -> Copying new files, please wait!"
    cp "$IMAGE" "$DIR"/Main_Page.png
    cp "$IMAGE" "$DIR"/Main_Page2.png
    # ... and so on ...

    echo " -> All background images created!"
    echo " -> Now copying to project folder!"
    cp "$DIR"/* "$DSTDIR"
else
    echo " -> Could not find new image to use, skipping!"
fi

echo "Help Pages:"
echo " -> Now creating Help logos"
IMAGE="$SRCDIR"/New_Help.png
if [ -s "$IMAGE" ] 
then
    echo " -> Found new file to be used!"
    DIR="$WORKDIR"/Help
    prepare_dir "$DIR"

    echo " -> Copying new files, please wait!"
    cp "$IMAGE" "$DIR"/Help_Page.png
    cp "$IMAGE" "$DIR"/Help_Page2.png
    # ... and so on ...

    echo " -> Applying changes to project!"
    cp "$DIR"/* "$DSTDIR"
else
    echo " -> Could not find new image to use, skipping!"
fi

echo "Icon Pages:"
IMAGE="$SRCDIR"/New_Icon.png
if [ -s "$IMAGE" ] 
then
    echo " -> Found new file to be used!"
    DIR="$WORKDIR"/Icon
    prepare_dir "$DIR"

    echo " -> Copying new files, please wait!"
    cp "$IMAGE" "$DIR"/Icon_Page.png
    cp "$IMAGE" "$DIR"/Icon_Page2.png

    echo " -> Applying changes to project!"
    cp "$DIR"/* "$DSTDIR"
else
    echo " -> Could not find new image to use, skipping!"
fi

echo "Additional Logos:"
echo " -> Now creating Logo"
IMAGE="$SRCDIR"/New_Logo.png
if [ -s "$IMAGE" ] 
then
    echo " -> Found new file to be used!"
    DIR="$WORKDIR"/Logo
    prepare_dir "$DIR"

    echo " -> Copying new files, please wait!"
    cp "$IMAGE" "$DIR"/Logo2.png

    echo " -> Applying changes to project!"
    cp "$DIR"/* "$DSTDIR"
else
    echo " -> Could not find new image to use, skipping!"
fi

echo "Boot Logos:"
echo " -> Now creating boot logos"
IMAGE="$SRCDIR"/New_Boot.png
if [ -s "$IMAGE" ] 
then
    echo " -> Found new file to be used!"
    DIR="$WORKDIR"/Boot
    prepare_dir "$DIR"

    echo " -> Copying new files, please wait!"
    cp "$IMAGE" "$DIR"/loading_screen.png

    echo " -> Applying changes to project!"
    cp "$DIR"/* "$DSTDIR"/additional_files
else
    echo " -> Could not find new image to use, skipping!"
fi

setbootlogo "$DSTDIR"/additional_files/loading_screen.png

echo "Wrapping up:"
# Fully tidy up, by removing any un needed file paths
echo " -> Removing un-needed files!"
rm -rf "$WORKDIR"/Main/*
rm -rf "$WORKDIR"/Boot/*
rm -rf "$WORKDIR"/Help/*
rm -rf "$WORKDIR"/Icon/*
rm -rf "$WORKDIR"/Logo/*
echo " -> Removing un-needed directories!"
rmdir "$DSTDIR"/Main
rmdir "$DSTDIR"/Boot
rmdir "$DSTDIR"/Help
rmdir "$DSTDIR"/Icon
rmdir "$DSTDIR"/Logo

echo "
****Project Customisation Script COMPLETE****
"