Use indentation to make the flow of logic easier to see
With indentation added to the original code,
it's a lot easier to understand the branches of the logic:
killall -9 omxplayer.bin
SERVICE='omxplayer'
while true; do
if ps ax | grep -v grep | grep $SERVICE > /dev/null
then
echo "Running" # sleep 5
else
sleep 5
log=log_file_pette.txt
# create log file or overrite if already present
printf "Log File Streaming- " >> $log
# append date to log file
date >> $log
# send email with date restart
echo `date` | mailx -s "Subject: Start streaming Ad" [email protected]
# if there is no internet and no ping
if ping -q -c 1 -W 1 google.com >/dev/null
then
killall -9 omxplayer.bin
$SERVICE -o alsa https://xxxxxxxxxxxx/playlist.m3u8 &
else
# create log file or overrite if already present
printf "Log File Local- " >> $log
# append date to log file
date >> $log
killall -9 omxplayer.bin
$SERVICE -o alsa looptv.mp4
sleep 5
fi
fi
done
Use functions to avoid duplicated logic
This snippet appears twice, with one word replaced:
# create log file or overrite if already present
printf "Log File Streaming- " >> $log
# append date to log file
date >> $log
It would be good to create a helper function for it, with the changing part a variable:
log_with_label() {
local label=$1
echo "Log File $label- $(date)" >> "$log"
}
Then you can call this with log_with_label "Streaming"
and log_with_label "Local"
.
Even the simple killall -9 omxplayer.bin
would be good in a helper function:
kill_player() {
killall -9 omxplayer.bin
}
The comments... are lying...
Some of the comments are confusing, because they don't tell the truth.
No sleeping happens here:
echo "Running" # sleep 5
The file will not be overwritten here. This will append to the file if exists:
# create log file or overrite if already present
printf "Log File Streaming- " >> $log
Use double-quotes around variables in shell commands
Instead of these:
if ps ax | grep -v grep | grep $SERVICE > /dev/null
printf "Log File Streaming- " >> $log
$SERVICE -o alsa https://xxxxxxxxxxxx/playlist.m3u8 &
write like this:
if ps ax | grep -v grep | grep "$SERVICE" > /dev/null
printf "Log File Streaming- " >> "$log"
"$SERVICE" -o alsa https://xxxxxxxxxxxx/playlist.m3u8 &
Avoid deeply nested code
Looking at this code:
while true; do
if ps ax | grep -v grep | grep $SERVICE > /dev/null
then
echo "Running" # sleep 5
else
# ... many, many lines here....
fi
done
By the time the I reach the fi
, I don't remember what the base condition was.
It would be good to reduce the nesting with a continue
in the if
branch.
(I'm wondering if you wanted to put a sleep 5
in there, in code, instead of a comment...)
while true; do
if ps ax | grep -v grep | grep $SERVICE > /dev/null
then
echo "Running" # sleep 5
continue
fi
# ... many, many lines here....
done
shellcheck.net is a that finds bugs in your shell scripts.
It's good to fix all the violations raised by it.
[email protected]
is a real email address, then I suggest to redact it. \$\endgroup\$