Take the 2-minute tour ×
Code Review Stack Exchange is a question and answer site for peer programmer code reviews. It's 100% free, no registration required.

This is a simple loop menu script that I have for class. The goal was to make a menu with applicable commands.

Menu:

  • A. Greet me. (Greet the user by their username (using the whoami command with back ticks))
  • B. Create a file in my home directory. (Ask the user what they want to name the file. Make sure the file does not already exist, using an if statement)
  • C. Search for a file. (Ask the user for the starting location of the file and the name. Use the find command)
  • D. Exit

I have 0 confidence in my scripting skills as I'm learning, so could you help me out? I don't know if put ;; at the end of choices like touch filescript;; read filescript;;, so on. Also, I'm not very sure of my choice C, creating in home directory.

#!/bin/bash
#
# This is Script 2 written by Jake Smoak
#
#
CHOICE=7
until [ $CHOICE -eq 4 ]
do
clear
echo Please select a menu item
echo
echo "A.) "Greet Me"
echo "B.) "Create a file in my home directory"
echo "C.) "Search for a file"
echo "D.) "Exit"
echo
read CHOICE
case $CHOICE in
A) echo "Hello $(whoami)."
B) echo "What would like to name the file?"
touch FILENAME;;
if [ -a $FILENAME ]
then 
echo "Sorry, this file is already made"
C) echo -n "Enter the location where the search should start. "
read STARTLOCATION
echo -n "What is the name of the file to search for? "
read FILENAME
echo "Search starting for the $FILENAME file in the $STARTLOCATION directory"
find $STARTLOCATION -name $FILENAME 2> /dev/null
D) echo "Sorry to see you go :("
*) echo You have made a mistake, Please try again"
esac
echo "Press Enter to continue"
read Enter
done
share|improve this question
    
is the lack of indenting in your question's code also missing in your actual code? –  rolfl Feb 10 at 22:38
    
In fact, your code is broken, it has out-of-place and non-matching " quotes, and it is missing fi statement to close an if condition.... does your code work? No! –  rolfl Feb 10 at 22:44
add comment

closed as off-topic by rolfl, syb0rg, James Khoury, palacsint, Jamal Feb 10 at 23:36

This question appears to be off-topic. The users who voted to close gave this specific reason:

  • "Your question must contain working code for us to review it here. For questions regarding specific problems encountered while coding, try Stack Overflow. After getting your code to work, you may edit this question seeking a review of your working code." – rolfl, syb0rg, James Khoury, palacsint, Jamal
If this question can be reworded to fit the rules in the help center, please edit the question.

1 Answer

As mentioned in the comments, there are some pretty major bugs (missing fi, etc). Here are some tips while you work on those:

You'd do well to break out the script into functions. Structure can make it easier to spot bugs. If you have

function greet() {
    echo "Hello $(whoami)"
}

Then your case for that can be simplified to

A)
    greet
;;

Which is obviously trivial here but can be more useful for the more complex clauses.

The ';;' is used to terminate an option in a case statement. It's generally placed on a line by itself for even better clarity. Speaking of which, when you have control statements like if/fi or case/esac, it's a good idea to indent the contents as well.

You should also be very careful about variable expansion.

find $STARTLOCATION -name $FILENAME 2> /dev/null

Will do bad things if my FILENAME is

secret-important-files -exec rm {} ;

So quote your arguments

find "$STARTLOCATION" -name "$FILENAME" 2> /dev/null
share|improve this answer
    
Thank you MR. Howard! You're very thorough and straight to the point! Cool guy :D –  user3294840 Feb 11 at 2:58
add comment

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