Code Review Stack Exchange is a question and answer site for peer programmer code reviews. Join them; it only takes a minute:

Sign up
Here's how it works:
  1. Anybody can ask a question
  2. Anybody can answer
  3. The best answers are voted up and rise to the top

Would the following be the right way to do it in your opinion? Can you suggest a better alternative or, if not, is there something that can be done to make the existing version better?

ps -e | grep skype | cut -d" " -f1 | xargs kill -s term
share|improve this question
    
I would install Linux-style killall to take care of this. – chicks Jul 19 at 22:21
1  
@chicks In Linux, /usr/bin/pkill is in the same procps package as /bin/ps, but /usr/bin/killall is in a separate psmisc package. Also, fun fact: on Solaris, killall actually tries to kill everything that it possibly can, regardless of command-line parameters! For both reasons, I recommend pkill over killall. – 200_success Jul 19 at 22:30
up vote 1 down vote accepted

As the comments have pointed out, there exist tools to match processes by name. This is roughly equivalent (better than) what you're trying to do:

pkill skype

It's better because:

  1. It's shorter and cleaner
  2. It won't kill itself

By the second point I mean that ps -e | grep skype will match the grep process itself too. That won't happen when using pkill.

Code review

If we wanted to imitate pkill, I would suggest writing like this:

ps -e | awk '/[s]kype/ { print $1 }' | xargs kill

That is:

  • Replace two processes with one (grep + cut with awk)
  • Use [s]kype instead of skype is a common trick to make the awk itself not match
  • Drop -s term from kill, as TERM is the default signal anyway
share|improve this answer

Your Answer

 
discard

By posting your answer, you agree to the privacy policy and terms of service.

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