Ompit: Critique my Bash script please~

Google · 2011-06-06 16:28:32

Ompit, it's my screen shot utility that automatically uploads to ompldr and copies the link to your clipboards.

I am pretty new to Bash. I basically dug around Google and looking at basic structures and examples and winged it all together. I think it's pretty useful and has some potential to be expanded.

I have been thinking of adding an argument to upload a picture without taking the scrot image. Any other thoughts? Be nice.

fsckd · 2011-06-06 17:01:04

Hash bang has to be the first line; move it to the top. As it is now, the kernel knows it is a script but not which shell it belongs to. So it's run using the default shell which is not guaranteed to be bash.

Google · 2011-06-06 17:06:59

Fixed, thanks!

demian · 2011-06-06 17:13:36

Well, i'm far from being a bash expert but here are my 2cents nevertheless .

1. You don't need root privileges for what you are doing in that script. But if you would need root privileges [ "$UID" -ne 0 ] is enough. You don't have to create a variable for the root id.

2. I'm not sure what you want the $1 in scrot $1 /tmp/screen.png for. If you want to be able to pass arguments to scrot i think it's better to use "$@". With $1 just the first argument after ompit will get passed to scrot. With "$@" everything after ompit will get passed as a block, including spaces.

3. You can replace

  if [ -e /tmp/screen.png ]; then
    rm -rf /tmp/screen.png
  fi

with [ -e /tmp/screen.png ] && rm -rf /tmp/screen.png
And while you're at it, you can wrap /tmp/screen.png as a variable. It looks prettier and if someone ever wanted to change the value it'd be easier for him that way.

The idea of ompit is good though. Kinda like wgetpaste. For inspiration, I suggest you look there. But one thing i would enjoy and could probably easily added would be to also create a thumbnail of the scrot image and upload it too.

Last edited by demian (2011-06-06 17:22:37)

Google · 2011-06-06 17:38:59

Thanks, I took those tips and I added a feature. I added the ability to specify a file path on the command line (file won't be deleted after the upload). I suppose you can also upload other files besides an image-- it should work for any.

edit:

I suppose I should check if the user specified file is in fact a file that exists...

Last edited by Google (2011-06-06 17:40:03)

hbekel · 2011-06-06 17:53:22

Some random hints:

Don't use [ if you're only using bash, see http://mywiki.wooledge.org/BashFAQ/031

Always quote your "$variables" unless you're sure they're just one word, see http://mywiki.wooledge.org/BashPitfalls … _.24target

Use the buildin type command instead of "which" (no need to use an external program for this).

In general, the mentioned wiki is a good starting point for bash programming, especially the "pitfalls" page.

Google · 2011-06-06 17:56:05

Thanks! That answers one of the problems I had discovered (file paths with spaces). That makes sense-- I will have to do some more reading. Thanks for the links~

edit:

I changed which for type-- I quickly did it and not sure if it's full-proof. If you see any errors please let me know.

edit 2:

Question, should I add in a portion in the block where a program isn't found-- it can check the distribution and then call the appropriate package manager to install the missing dependency? Or leave it up to the user? Which is generally common or better looked at?

Last edited by Google (2011-06-06 18:17:27)

demian · 2011-06-06 19:27:50

KISS

Google · 2011-06-07 11:49:34

Thanks-- I will do!

MrX · 2011-06-11 10:35:35

Replace

screen_path="/tmp/screen.png"

with

screen_path="`mktemp`"

to avoid conflicts when multiple instances of the script are running at the same time.

fumbles · 2011-06-11 13:44:42

MrX wrote:

Replace
screen_path="/tmp/screen.png"
with
screen_path="`mktemp`"
to avoid conflicts when multiple instances of the script are running at the same time.

... And better yet, instead of using `command` use $(command). At the very least it is easier to read.

Google · 2011-06-13 03:30:35

Thanks-- I will make a new commit in a day or so.

jnguyen · 2011-06-13 08:13:55

Give some useful exit values: http://mywiki.wooledge.org/BashGuide/Te … xit_Status

Use $(command) rather than `command`. Backticks are deprecated: http://mywiki.wooledge.org/BashFAQ/082

You probably want to make sure something useful happens if "rm -rf $screen_path" failed (e.g. insufficient permissions).

Use "grep -E" instead of "egrep". They are the same, but egrep is deprecated.

Google · 2011-07-06 03:55:37

Thanks-- sorry for the slow reply. I updated the script. I still need to update the script on Git because I reinstalled Arch and need to setup Git again.

I will try to update Git after work~

Thanks and if there's any more tips please let me know!

RyanMcCoskrie · 2011-07-19 04:24:29

I'm pretty sure that rm doesn't need the '-r' option to remove the file. I'm only speaking from a quick glance, but it struck me as odd that you would use the recursive option on a file that you know is not a directory. Feel free to ignore me if I missed something of course.

Google · 2011-07-26 23:38:05

RyanMcCoskrie wrote:

I'm pretty sure that rm doesn't need the '-r' option to remove the file. I'm only speaking from a quick glance, but it struck me as odd that you would use the recursive option on a file that you know is not a directory. Feel free to ignore me if I missed something of course.

Sorry for the slow reply. I removed the r flag but kept the f flag. I also moved the source over to bitbucket.

Arch Linux

#1 2011-06-06 16:28:32

Ompit: Critique my Bash script please~

#2 2011-06-06 17:01:04

Re: Ompit: Critique my Bash script please~

#3 2011-06-06 17:06:59

Re: Ompit: Critique my Bash script please~

#4 2011-06-06 17:13:36

Re: Ompit: Critique my Bash script please~

#5 2011-06-06 17:38:59

Re: Ompit: Critique my Bash script please~

#6 2011-06-06 17:53:22

Re: Ompit: Critique my Bash script please~

#7 2011-06-06 17:56:05

Re: Ompit: Critique my Bash script please~

#8 2011-06-06 19:27:50

Re: Ompit: Critique my Bash script please~

#9 2011-06-07 11:49:34

Re: Ompit: Critique my Bash script please~

#10 2011-06-11 10:35:35

Re: Ompit: Critique my Bash script please~

#11 2011-06-11 13:44:42

Re: Ompit: Critique my Bash script please~

#12 2011-06-13 03:30:35

Re: Ompit: Critique my Bash script please~

#13 2011-06-13 08:13:55

Re: Ompit: Critique my Bash script please~

#14 2011-07-06 03:55:37

Re: Ompit: Critique my Bash script please~

#15 2011-07-19 04:24:29

Re: Ompit: Critique my Bash script please~

#16 2011-07-26 23:38:05

Re: Ompit: Critique my Bash script please~

Board footer