Working Script Peer Review?

[Matthew Kruer]

Just a peer review

This script works, but I want to know if there is a more elegant way of doing it.


Thanks



#!/bin/bash
#

CONSOLEOUT="%-50s"

default_hostname1() {
HOSTNAME_1=`hostname -s`
}

#Set Defaults
default_hostname1

while [[ $CORRECT != "y" ]]
do
#### Enter First Hostname ###
if [[ -n $HOSTNAME_1 ]] ; then
DEFAULT_HOSTNAME_1="$HOSTNAME_1"
FORMATED_DEFAULT_HOSTNAME_1="[$HOSTNAME_1]"
fi

printf "$CONSOLEOUT" "Enter First Hostname $FORMATED_DEFAULT_HOSTNAME_1"
read HOSTNAME_1
if [[ -n $DEFAULT_HOSTNAME_1 ]] && [[ -z $HOSTNAME_1 ]] ; then
HOSTNAME_1=$DEFAULT_HOSTNAME_1
fi

#### Enter Second Hostname ###
if [[ -n $HOSTNAME_2 ]] ; then
DEFAULT_HOSTNAME_2="$HOSTNAME_2"
FORMATED_DEFAULT_HOSTNAME_2="[$HOSTNAME_2]"
fi

printf "$CONSOLEOUT" "Enter Second Hostname $FORMATED_DEFAULT_HOSTNAME_2"
read HOSTNAME_2
if [[ -n $DEFAULT_HOSTNAME_2 ]] && [[ -z $HOSTNAME_2 ]] ; then
HOSTNAME_2=$DEFAULT_HOSTNAME_2
fi

while [[ -z $HOSTNAME_2 ]]
do
printf "$CONSOLEOUT" "Invalid Hostname, Enter Second Hostname"
read HOSTNAME_2
done

### Confirm ####
echo "Host 1 is $HOSTNAME_1"
echo "Host 2 is $HOSTNAME_2"
echo "Is this correct? [y/n]"
read CORRECT
done

[Ed Morton]

Seems fine, but I'd probably make a function out of the common parts for the 2
HOSTNAMEs, see below.

#!/bin/bash
#

CONSOLEOUT="%-50s"

default_hostname() {
HOSTNAME[$1]=`hostname -s`
}

get_hostname() {
if [[ -n ${HOSTNAME[$1]} ]] ; then
DEFAULT_HOSTNAME[$1]="${HOSTNAME[$1]}"
FORMATED_DEFAULT_HOSTNAME[$1]="[${HOSTNAME[$1]}]"
fi

printf "$CONSOLEOUT" "Enter Hostname $1 [${HOSTNAME[$1]}]"
read HOSTNAME[$1]
if [[ -n ${DEFAULT_HOSTNAME[$1]} ]] && [[ -z ${HOSTNAME[$1]} ]] ; then
HOSTNAME[$1]=${DEFAULT_HOSTNAME[$1]}
fi
}

#Set Defaults
default_hostname 1

while [[ $CORRECT != "y" ]]
do
#### Enter First Hostname ###

get_hostname 1

#### Enter Second Hostname ###

get_hostname 2

while [[ -z ${HOSTNAME[2]} ]]

do
printf "$CONSOLEOUT" "Invalid Hostname, Enter Second Hostname"

read HOSTNAME[2]
done

### Confirm ####
echo "Host 1 is ${HOSTNAME[1]}"
echo "Host 2 is ${HOSTNAME[2]}"

echo "Is this correct? [y/n]"
read CORRECT
done

I don't really follow your logic, though, in the way you have a default for host
1 but not host 2 and then you have a separate loop to populate host 2 after you
try to get it. There may be a simpler solution.

Ed.

[Matthew Kruer]

Thanks for taking the time to look at the script.

This script will play a small part in a larger script. Right now I have virtually no error handling so I was tinkering on trying to make it more idiot proof.

Hostname2 is what I am calling it its actually a second system and the system will attempt to query the remote system.

I guess the logic was when the user first runs the script, it gathers hostname of the current system (realistically the system name would not change while running the script, however there is the possibility that they want to change the name from just the hostname to the hostname+domain. ) So the first time they might bypass the hostname accept the default and then for the second name enter the host+domain. Upon conformation they could go back a change either. I don’t suppose there is a good way to echo out the values to the read line? So if all they need to do is change the number, it’s their to hit backspace and change the character and hit enter to update? Doing it that way might get rid of the need of the formatted version of the value.

[Ed Morton]

On 8/23/2012 11:34 PM, Matthew Kruer wrote:
<snip>

> I don’t suppose there is a good way to echo out the values to the read line? So if all they need to do is change the number, it’s their to hit backspace and change the character and hit enter to update? Doing it that way might get rid of the need of the formatted version of the value.

I think I've seen a CLI that did something like that implemented using "curses"
(http://en.wikipedia.org/wiki/Curses_%28programming_library%29). Seems like it'd
be more trouble than it's worth, though, and maybe there's some more modern
alternative.

Ed.

[Ed Morton]

On 8/23/2012 11:34 PM, Matthew Kruer wrote:

> Thanks for taking the time to look at the script.
>
> This script will play a small part in a larger script. Right now I have virtually no error handling so I was tinkering on trying to make it more idiot proof.
>
> Hostname2 is what I am calling it its actually a second system and the system will attempt to query the remote system.
>

> I guess the logic was when the user first runs the script, it gathers hostname of the current system (realistically the system name would not change while running the script, however there is the possibility that they want to change the name from just the hostname to the hostname+domain. ) So the first time they might bypass the hostname accept the default and then for the second name enter the host+domain. Upon conformation they could go back a change either. I don�t suppose there is a good way to echo out the values to the read line? So if all they need to do is change the number, it�s their to hit backspace and change the character and hit enter to update? Doing it that way might get rid of the need of the formatted version of the value.
>

Ah, I see. Then, sticking to your original style as much as possible, I'd write
it as:

#!/bin/bash

get_hostname() {
do
printf "Enter Hostname %d %-43s" "$1" "[${hostname[$1]}]" >&2
read input_hostname
if [[ -n $input_hostname ]]; then
hostname[$1]="$input_hostname"
fi
if [[ -z ${hostname[$1]} ]]; then
printf "ERROR: Hostname $1 is required.\n" >&2
fi
until [[ -n ${hostname[$1]} ]]
}

#Set Default
hostname[1]="$(hostname -s)"

do
#### Enter First Hostname ###
get_hostname 1

#### Enter Second Hostname ###
get_hostname 2

### Confirm ####
printf "Host 1 is %s\n" "${hostname[1]}" >&2
printf "Host 2 is %s\n" "${hostname[2]}" >&2
printf "Is this correct? [y/n]" >&2
read correct

until [[ $correct = "y" ]]

Notice I changed all your variables to lower case since all-upper-case by
convention is for exported variables only.

Regards,

Ed.

[Thomas 'PointedEars' Lahn]

1. Curses is _not_ obsolete. dialog(1) and other tools use ncurses.
2. Since it's bash, it's very simple: read -e -i 'bar' -p 'foo: '

RTFM.

--
PointedEars

Twitter: @PointedEars2
Please do not Cc: me. / Bitte keine Kopien per E-Mail.