Was this shell script ready for open-source? (Random XKCD wallpaper)

Question

I wrote the following little shell script and published it on GitHub. In the repo there's also a README file that explains how to use it in more detail.

This is the first time I open-sourced/published something I wrote, so I'm interested whether you think that this code is ready for the public.

Some pointers, but feel free to add your own thoughts:

1. Does it look professional? If not, what would need to be done?
2. Are the comments too much? Was it ok to publish it, although I know that under some circumstances it doesn't work (I wrote about that in the README)?

#    random xkcd wallpaper: gets a random xkcd comic from xkcd.com and sets it as the desktop background.


#!/bin/bash



# /random/comic redirects to random comic. Wget gets the index.html of the comic.
wget http://dynamic.xkcd.com/random/comic/

echo $(pwd)

#Searches the line in the index.html file that points to the url where the actual comic is placed.
#The image urls are of the form: http://imgs.xkcd.com/comics/.'name of comic'.(png | jpg)
url=$(cat index.html | grep -o -m 1 http://imgs.xkcd.com/comics/.*\.png)

#Assuming picture format is .png. Gets the name of the image file by only matching what comes after the last forward slash.
name_pic=$(echo $url | grep -o [^/]*\.png)
is_png=1


#Sets url and name_pic in the case of the picture being in .jpg format.
if [ -z "$url" ]
then
    url=$(cat index.html | grep -o -m 1 http://imgs.xkcd.com/comics/.*\.jpg)
    name_pic=$(echo $url | grep -o [^/]*\.jpg)
    is_png=0   
fi     


#Downloads the image and saves it under its appropriate name.
wget --output-document="$name_pic"  "$url"

#Sets the desktop background
gconftool-2 --set --type=string /desktop/gnome/background/picture_filename $(pwd)/"$name_pic"

#Cleans up
rm index.html

#For some reason, if the image is moved to fast (e.g without a wait) the background does not get set.
sleep 1

#The current wallpaper can always be found under "current_xkcd_wallpaper.png" or "current_xkcd_wallpaper.jpg". Also prevents cluttering the directory with images. 
#If you want to keep all the pictures you downloaded, uncomment the following lines.

#Cleans up. Makes sure that there is only one current_xkcd_wallpaper image file.
#rm current_xkcd_wallpaper.*

rm current_xkcd_wallpaper.*

if [ $is_png = 1 ] ; then
    mv $(pwd)/"$name_pic" $(pwd)/current_xkcd_wallpaper.png
else
    mv $(pwd)/"$name_pic" $(pwd)/current_xkcd_wallpaper.jpg
fi

Answer 1

Sorry, this might sound harsh, but I really think GPLv3 is plain overkill for your code. And this even might sound harsher: What you want to archive can be done in a 3-liner, so I think Public Domain would fit far better.

I do not want to show you up as everybody starts as beginner, and basically it's your idea which counts, the implementation itself is mostly an exercise for the reader. So continue learning shell, it's tremendously powerful!

An elaborated example which basically does what your script does, it borrows some details from your script:

#!/bin/bash

cd "`dirname "$0"`" || exit

[ 0 = "$#" ] &&
set -- gconftool-2 --set --type=string /desktop/gnome/background/picture_filename

url="`curl -sL http://dynamic.xkcd.com/random/comic/ | grep -om1 'http://imgs.xkcd.com/comics/[^.]*\.[a-z]*'`"
img="$PWD/xkcd-wallpaper.${url##*.}"

curl -so "$img" --fail "$url" &&
"$@" "$img"

I wrapped the lines just for better readability. Note that every programmer has a set of favorite tools and ways, and one always can learn something from each other. Like I learned "grep -o" from your script today (I usually use sed, but grep -o is far more elegant in this context).

The nice thing on Unix is that you can chain things so easily and flexibly. There is no need to squeeze everything into one monolithic script. The better way would be to create two scripts as building blocks and then join them together.

One pulls the picture. Second installs the picture on gnome. Like this:

xkcd-pull.sh:

#!/bin/bash

url="`curl -sL http://dynamic.xkcd.com/random/comic/ | grep -om1 'http://imgs.xkcd.com/comics/[^.]*\.[a-z]*'`"

img="/tmp/xkcd-wallpaper.${url##*.}"
img="${1:-$img}"

curl -so "$img" --fail "$url" && echo "$img"

set-gnome-wp.sh

#!/bin/bash

exec gconftool-2 --set --type=string /desktop/gnome/background/picture_filename "$@"

And then join that together, for example on the commandline:

img="`./xkcd-pull.sh`" && ./set-gnome-wp.sh "$img"

Following this pattern you can easily extend it for KDE or perhaps even adapt it to Cygwin under Windows. Please note that these snippets even contain some extra bloat, which might come handy later on.

As you can see easily, doing it this way there is nothing left which you cannot look up in a manual. So all creativity left is your idea "XKCD -> Wallpaper" while the implementation is "trivial".

(I did not test the code snippets here. Maybe there still is a typo in it. My contributed code is Public Domain.)

Answer 2

#!/bin/bash

The shebang line has no effect unless it's on the very first line, so you should put it before the copyright notice.

echo $(pwd)

echo $(command) is almost always the same as just writing command (the exception being that echo adds a newline at the end if the command doesn't already produce one, but that's not the case here). So you can just change the above to pwd.

cat index.html | grep -o -m 1 http://imgs.xkcd.com/comics/.*\.png

That's useless use of cat. If you want to grep through a file, you can just pass the filename as the last argument - no need to cat and pipe. (The same goes for later places in the code where you do the same with .jpg).

gconftool-2 --set --type=string /desktop/gnome/background/picture_filename $(pwd)/"$name_pic"

I think it'd be a good idea to make the command to change the wallpaper configurable, so non-GNOME users can use your script too.

Also I'm not sure why you set the image to be the wallpaper before renaming it. I think it'd make more sense to do so afterwards (that might also solve your issue of having to sleep).

---

On a general note you might take care to gracefully handle already existing files. For example if the script is called in a directory where an index.html already exists (which is not that unlikely), your script will a) not work and b) delete that index.html, which its owner might not appreciate.

Answer 3

As well as the other problems already outlined, your code gets a file 'index.html', which could clobber any file of the same name in the current directory. Maybe it would be a good idea to create a new directory so that you don't mess things up?

It would also be a good idea to make sure that the debris is removed even if the script is interrupted. You do that with 'trap'. I recommend trapping HUP, INT, QUIT, PIPE and TERM signals:

trap "rm -f index.html; exit 1" 0 1 2 3 13 15

...other actions...

rm -f index.html
trap 0

The trap 0 at the end ensures that your script can exit successfully - very important. You can make your trap actions as complex as necessary - and should aim to keep them as simple as possible. One advantage of creating a directory for intermediate files is that you can simply remove that directory to clean up.