3
\$\begingroup\$
#!/bin/bash
#  Used to add/remove printers from system (lp)
#  Copyright Fresh Computer Systems Pty Ltd.
GREP_OPTIONS='--color=tty'
export GREP_OPTIONS

sanity() {
    #  Are we root?
    if [ $EUID != '0' ]; then
        exitError "You must be root to run this script." 1
    fi

    if [ ! -x /usr/sbin/lpadmin -a ! -x /sbin/lpadmin ]; then
        #  debug
        #printf " @@@ ERROR: \'lpadmin\' was not found, or is not executable. Quitting.\n\n"
        exitError " @@@ ERROR: \'lpadmin\' was not found, or is not executable" 1
    fi

    PPD="/usr/share/cups/model/postscript.ppd"
    PPDGZ="/usr/share/cups/model/postscript.ppd.gz"
    if [ -f $PPD ]; then
        POSTSCRIPT="$PPD"
    elif [ -f $PPDGZ ]; then
        POSTSCRIPT="$PPDGZ"
    else
        exitError " @@@ ERROR: No postscript file found. Please ensure there is a \"$PPD\" or a \"$PPDGZ\" file." 1
    fi

    #  ensure sbin is in path
    PATH=/usr/sbin:/sbin:${PATH}
    # /etc/hosts file
    HOSTS="/etc/hosts"
    if [ ! -w "$HOSTS" ]; then
        exitError "Hosts file \"$HOSTS\" is not writeable." 1
    fi
}

exitError() {
    #  Provide default exit code '1' if none provided
    if [ -n "$1" ]; then
        ERRMSG="$1"
        ERRCODE="${2-1}"
    else
        ERRMSG=" @@@ ERROR: Unspecified Error."
        ERRCODE="${1-1}"
    fi
    ERRMSG="$ERRMSG"
    printf "\n%s\n" "$ERRMSG"   >&2
    printf "%s\n\n" "Quitting." >&2
    exit ${ERRCODE}
}

invalidInput() {
    declare USELESS
    printf "\nInvalid input.\n" >&2
    read -p "Press Any Key To Continue..." USELESS
    #unset USELESS
    printf "\n"
}

##
##  FUNCTIONS
##

enterPrinterName() {
    unset PRINTER_TYPE PRINTER_NAME IPADDRESS IPADDRESS_SED CONNECTION_TYPE REPLY RESULT PORTNAME NAME_EXISTS_IN_HOSTS ADDRESS_EXISTS_IN_HOSTS HOSTACTION RC
    printf "\n"
    while true; do
        read -p "Enter printer name, or [q]uit:  " PRINTER_NAME
        case $PRINTER_NAME in
           [qQ])    exitError "Aborted by User" 0
                    ;;
            ??*)    break
                    ;;
           ""|*)    invalidInput        #blank line or anything else
                    ;;
        esac
    done
}

checkPrinterExistsInSystem () {
    if lpstat -v $PRINTER_NAME 1>/dev/null 2>&1; then
        #  Printer already exists in the system
        #lpstat -v $PRINTER_NAME
        #printf "Printer already exists in system!\n" >&2
        declare EXISTS=0      #true
    else
        declare EXISTS=1      #false
    fi
    printf "\n"
    NAME_EXISTS_IN_SYSTEM=$EXISTS
    return $EXISTS
}

checkNameExistsInHosts() {
    declare EXISTS=1
    #  scan each line from $HOSTS
    while read LINE; do
        #  Remove comments
        LINE="${LINE%%#*}"
        case "$LINE" in
            *${PRINTER_NAME}*)
                #  printer found
                EXISTS=0
                break #stop looping
                ;;
            *)
                #  printer not found
                :
                ;;
        esac
    done < $HOSTS

    if [ $EXISTS -eq 0 ]; then
        printf "Printer \"$PRINTER_NAME\" already exists in \"$HOSTS\"!\n\n" >&2
        egrep "^[^#]*$PRINTER_NAME[[:space:]]*" "${HOSTS}"
    fi

    printf "\n"
    NAME_EXISTS_IN_HOSTS=$EXISTS
    return $EXISTS
}

checkAddressExistsInHosts() {
    declare EXISTS=1
    while read LINE; do
        #  Remove comments
        LINE="${LINE%%#*}"
        case "$LINE" in
            *${IPADDRESS}*)
                #  address exists in hosts
                EXISTS=0      #true
                break
                ;;
            *)
                #  address does not exist
                :
                ;;
        esac
    done < $HOSTS

    if [ $EXISTS -eq 0 ]; then
        #  address exists, add alias
        #echo "............. HERE I AM 1 .............. "
        HOSTACTION=addAliasToHosts
        printf "IP address \"$IPADDRESS\" already exists in \"$HOSTS\"!\n\n"
        egrep "^[^#]*$IPADDRESS[[:space:]]*" "${HOSTS}"
    else
        #  address is NEW, add as a new host
        #echo "............. HERE I AM 2 .............. "
        HOSTACTION=addHostToHosts
    fi

    ADDRESS_EXISTS_IN_HOSTS=$EXISTS
    printf "\n"
    return $EXISTS
}

getConnectionType() {
    while true; do
        #  Connection type
        printf "\nCONNECTION TYPE:
        1. LPD (print server)
        2. 9100 (network printer)\n\n"
        read -p "Enter a number, or [q]uit:  " REPLY
        case $REPLY in
          [qQ]) exitError "Aborted by User." 0
                ;;
            1)  CONNECTION_TYPE="LPD"
                break
                ;;
            2)  CONNECTION_TYPE=9100
                break
                ;;
         ""|*)  invalidInput
                ;;
        esac
    done
    printf "Connection type selected: \"$CONNECTION_TYPE\".\n\n"
    if [ "$CONNECTION_TYPE" = "LPD" ]; then
        #  LPD connection requires a portname
        while true; do
            read -p "$CONNECTION_TYPE: give portname (e.g. p1), or [q]uit:  " REPLY
            case $REPLY in
                  [qQ]) exitError "Aborted by User." 0
                        ;;
          [[:alnum:]]*) PORTNAME="$REPLY"   #at least one alphanumeric char
                        break
                        ;;
                  ""|*) invalidInput
                        ;;
            esac
        done
        printf "OK: Portname selected: \"$PORTNAME\".\n\n"
    fi
}

getPrinterType() {
    while true; do
        #  Printer Type
        read -p "Is it a POSTSCRIPT printer, or [q]uit? [y/n/q]  "
        case $REPLY in
          [qQ])  exitError "Aborted by User." 0
                ;;
         [yY])  PRINTER_TYPE="postscript"
                break
                ;;
         [nN])  PRINTER_TYPE="raw"
                break
                ;;
         ""|*)  invalidInput
                ;;
        esac
    done
    printf "OK: Printer type selected: \"$PRINTER_TYPE\".\n\n"
}

getIpAddr() {
    while true; do
    read -p "Enter full IP address (e.g. 192.168.111.222) of printer \"$PRINTER_NAME\", or [q]uit:  " REPLY

        case $REPLY in
            [qQ])    exitError "Aborted by User." 0
                    ;;
        *.*.*.*)    IPADDRESS="$REPLY"      #no robust pattern checking...
                    break
                    ;;
           ""|*)    invalidInput
                    ;;
        esac
    done
    printf "OK: IP address selected: %s\n\n" "$IPADDRESS"
}

addHostToHosts() {
    if ! [ -w "$HOSTS" -a -n "$IPADDRESS" -a -n "$PRINTER_NAME" ]; then
        #  debug
        #printf "\$HOSTS = $HOSTS\n"
        #printf "\$IPADDRESS = $IPADDRESS\n"
        #printf "\$PRINTER_NAME = $PRINTER_NAME\n"
        exitError " @@@ ERROR: Error in function \"$FUNCNAME\" on line \"$LINENO\"." 1
    fi

    #  append host to bottom of $HOSTS
    printf "%-20s%s\n" "$IPADDRESS" "$PRINTER_NAME" >> "${HOSTS}"

    #  show result
    egrep "\<$IPADDRESS\>" "$HOSTS"
    ERROR=$?
    return $ERROR
}

addAliasToHosts() {
    if ! [ -w "${HOSTS}" -a -n "$IPADDRESS" -a -n "$PRINTER_NAME" ]; then
        exitError " @@@ ERROR: Error in function \"$FUNCNAME\" on line \"$LINENO\"." 1
    fi

    #  get IPADDRESS line in $HOSTS
    declare PRINTER_NAME_LINE=$(egrep "^[^#]*\<$IPADDRESS\>" "$HOSTS")

    ########
    #while read LINE; do
    #    #  Remove comments
    #    LINE="${LINE%%#*}"
    #    case "$LINE" in
    #        *${IPADDRESS}*)
    #            #  address found
    #            declare EXISTS=0
    #            break #stop looping
    #            ;;
    #        *)
    #            #  address not found
    #            declare EXISTS=1
    #            ;;
    #    esac
    #done < $HOSTS
    ########

    #  store data (aka remove comments)
    declare DATASTORE=${PRINTER_NAME_LINE%%#*}

    #  append new printer name to ip address
    declare NEW_PRINTER_NAME_LINE="$DATASTORE $PRINTER_NAME"

    #  store comments (aka remove data)
    case $PRINTER_NAME_LINE in
        *#*)    declare COMMENTSTORE=${PRINTER_NAME_LINE#*#}
                #  append alias to stored data
                declare NEW_PRINTER_NAME_LINE="$NEW_PRINTER_NAME_LINE#$COMMENTSTORE"
                ;;
    esac

    #  append alias to $HOSTS
    printf "Appending \"$PRINTER_NAME\" to \"$HOSTS\" as alias using IP address \"$IPADDRESS\".\n\n"
    sed -r -e "s/$PRINTER_NAME_LINE/$NEW_PRINTER_NAME_LINE/" -i $HOSTS
    RESULT=$?

    #  show result
    egrep "\<$IPADDRESS\>" "$HOSTS"
    return $RESULT
}

removeFromHosts() {
    #  grab line with printer name
    declare PRINTER_NAME_LINE=$(egrep "^[^#]*$PRINTER_NAME[[:space:]]*" ${HOSTS})
    #printf "\$PRINTER_NAME_LINE = $PRINTER_NAME_LINE\n"

    case "$PRINTER_NAME_LINE" in
          *#*)  #  save comment for later (aka delete data)
                declare COMMENTSTORE="${PRINTER_NAME_LINE#*#}"
                declare HASCOMMENT=0 #true
                ;;
            *)  declare HASCOMMENT=1 #false
                ;;
    esac

    #  store data (aka remove all comments)
    declare DATASTORE="${PRINTER_NAME_LINE%%#*}"
    #printf "\$DATASTORE = $DATASTORE\n"

    #  delete PRINTER_NAME
    declare DATASTORE=${DATASTORE/$PRINTER_NAME}
    #printf "\$DATASTORE = $DATASTORE\n"

    backupHosts || exit

    #  Do we have a host remaining for this IP address?
    #  REGEX: ipaddress followed by one or more spaces, followed by one or more alphanumeric chars
    if egrep "^[^#]*[[:digit:]]{0,3}\.[[:digit:]]{0,3}\.[[:digit:]]{0,3}\.[[:digit:]]{0,3}[[:space:]]+[[:alnum:]]+" <<<"${DATASTORE}" >/dev/null 2>&1; then
        #  we still have a hostname/alias
        if [ $HASCOMMENT -eq 0 ]; then
            #  Comment present. Append it.
            declare DATASTORE="${DATASTORE}#${COMMENTSTORE}"
        fi

        #  BUG: TODO: this is erasing ALL occurences of printer_name
        #  should only modify the one we want
        #sed -r -e "s/$PRINTER_NAME_LINE[[:space:]]*/$NEW_PRINTER_NAME_LINE/g" -i "$HOSTS"

        sed -r -e "s/^[^#]*$PRINTER_NAME_LINE[[:space:]]*/$DATASTORE/g" -i "${HOSTS}"

        SED_RESULT=$?
    else
        #  we have no hostnames/aliases remaining. Delete entire line
        #printf " ... running sed ... \n"
        #printf "\$PRINTER_NAME_LINE = $PRINTER_NAME_LINE\n"
        #printf "\$DATASTORE = $DATASTORE\n"

        sed -r -e "/^[^#]*$PRINTER_NAME/d" -i "$HOSTS"
        SED_RESULT=$?
    fi

    if [ $SED_RESULT -eq 0 ]; then
        printf "Ok: Printer \"$PRINTER_NAME\" successfully removed from \"$HOSTS\".\n"
    else
        exitError " @@@ ERROR: Fatal error on line \"$LINENO\" in function \"$FUNCNAME\"." 1
    fi

    return $SED_RESULT
    #printf "...reached end of removeFromHosts...\n"
}

backupHosts () {
    printf "Making backup copy of \"$HOSTS\" to \"${HOSTS}.$$\".\n"
    #printf "cp -v \"${HOSTS}\" \"${HOSTS}.$$\"\n\n"
    cp "${HOSTS}" "${HOSTS}.$$"
    RESULT=$?
    if [ $RESULT -eq 0 -a -f "$HOSTS.$$" ]; then
        printf "Successfully backed up hosts file.\n\n"
    else
        printf "Backup of hosts file was not successful.\n\n" >&2
    fi
    return $RESULT
}

lpadd() {
    COMMANDLINE="lpadmin -p ${PRINTER_NAME} -E"

    if [ -n "${PRINTER_NAME}" -a -n "${PRINTER_TYPE}" -a -n "${CONNECTION_TYPE}" ]; then



        #  PRINTER_TYPE


        if [ "${PRINTER_TYPE}" = "raw" ]; then
            #  Printer Type: RAW
            COMMANDLINE="${COMMANDLINE} -m raw"

        elif [ "$PRINTER_TYPE" = "postscript" ]; then
            #  Printer type: postscript
            COMMANDLINE="${COMMANDLINE} -P $POSTSCRIPT"

        else
            exitError " @@@ ERROR: Invalid Printer Type: \"$PRINTER_TYPE\"." 1
        fi


        #  CONNECTION_TYPE


        if [ "$CONNECTION_TYPE" = "LPD" ]; then
            if [ -n "${PORTNAME}" ]; then
                COMMANDLINE="${COMMANDLINE} -v lpd://${PRINTER_NAME}/${PORTNAME}"
            else
                exitError "Error: Missing \$PORTNAME." 1
            fi

        elif [ "$CONNECTION_TYPE" = 9100 ]; then
            COMMANDLINE="${COMMANDLINE} -v socket://${PRINTER_NAME}:9100"

        else
            exitError " @@@ ERROR: Invalid Connection Type: \"$CONNECTION_TYPE\"." 1
        fi

    else
        # debug
        echo " -------------------------- *** ---------------------------"
        echo "----------------- SHOULD NEVER BE HERE ------------------"
        echo " -------------------------- *** ---------------------------"
        exitError " @@@ ERROR: Serious error. Insufficient values provided. Function \"$FUNCNAME.\"" 1
        # debug
        printf "printer name: %s\nprinter type: %s\nconnection type: %s\nportname: %s\n" \
            "$PRINTER_NAME" "$PRINTER_TYPE" "$CONNECTION_TYPE" "$PORTNAME"
    fi

    #  provide error policy
    COMMANDLINE="${COMMANDLINE} -o printer-error-policy=retry-job"

    while true; do
        printf "About to run this command: \033[1;31m%s\033[0m\n\n" "${COMMANDLINE}"
        read -p "Are you sure? 'yes' or [q]uit? [yes/q]  " REPLY
        case $REPLY in
           [qQ])    exitError "Aborted by User." 0
                    ;;
        yes|YES)    break #yes. doAddPrinter
                    ;;
           ""|*)    invalidInput
                    ;;
        esac
    done
}

doAddPrinter() {
    declare SUCCESS
    #  backup hosts file
    backupHosts || exit

    #if [ "$ADDTOHOSTS" = "alias" ] ; then
    #    addAliasToHosts
    #else
    #    addHostToHosts
    #fi

    #  add host, or alias
    #  debug
    #printf "\$HOSTACTION = $HOSTACTION\n"
    eval $HOSTACTION

    #  add printer
    printf "\n"
    eval $COMMANDLINE
    #if eval $COMMANDLINE ; then
    if checkPrinterExistsInSystem; then
        SUCCESS=0
        lpstat -v "$PRINTER_NAME"
        printf "Printer successfully added.\n\n"
    else
        SUCCESS=1
        printf "Printer was not successfully added!\n\n" >&2
        #exit
    fi
    return $SUCCESS
}

acceptPrinter () {
    printf "Enabling new printer \"$PRINTER_NAME\"...\n"
    if which accept >/dev/null 2>&1; then
        accept $PRINTER_NAME
    elif which cupsenable >/dev/null 2>&1; then
        cupsenable $PRINTER_NAME
    else
        printf "Could not enable printer. Try running 'accept $PRINTER_NAME' or 'cupsenable $PRINTER_NAME' manually.\n" >&2
        false
    fi
    RC=$?
    return $RC
}

lprm() {
    while true; do
        read -p "Are you sure you want to remove printer \"$PRINTER_NAME\"? [yes/q]:  " REPLY
        case $REPLY in
            [qQ])   exitError "Aborted by User." 0
                    ;;
         yes|YES)   break
                    ;;
            ""|*)   invalidInput
                    ;;
        esac
    done
    lpadmin -x "${PRINTER_NAME}"            #  delete printer
    if checkPrinterExistsInSystem; then     #  does printer still exist?
        #  printer remains -- bad
        declare SUCCESS=1     #false
    else
        #  printer deleted -- good
        declare SUCCESS=0     #true
        #FIXME
        removeFromHosts
    fi
    return $SUCCESS
}

main () {
    enterPrinterName
    if checkPrinterExistsInSystem; then
        lpstat -v ${PRINTER_NAME}
        printf "\nPrinter \"${PRINTER_NAME}\" already exists in the system.\n"
        while true; do
            read -p "[d]elete printer, [r]estart, or [q]uit? [r/s/q]?  " REPLY
            case $REPLY in
                [qQ])   exitError "Aborted by User." 0
                        ;;
                [dD])   if lprm "$PRINTER_NAME"; then
                            printf "OK: Printer \"${PRINTER_NAME}\" successfully removed from the system.\n\n"
                            #break       #call checkNameExistsInHosts
                            return 0
                        else
                            exitError " @@@ ERROR: Failure to remove printer from the system.\n" 1
                        fi
                        ;;
                [rR])   return 0    #exit main, start loop again, restart script
                        ;;
            esac
        done
    else
        # consider deleting completely
        : #printf "OK: Printer \"${PRINTER_NAME}\" does not exist in the system.\n\n"
    fi

    if checkNameExistsInHosts; then
        while true; do
            read -p "Are the details correct [y], [r]estart or [q]uit? [y/r/q]  " REPLY
            case $REPLY in
               [qQ])    exitError "Aborted by User." 0
                        ;;
               [yY])    #if ok -> getData()
                        break
                        ;;
               [rR])    #if not ok -> loop back main
                        printf "OK: Restarting script.\n\n"
                        return 0 #quit main and restart script
                        ;;
               ""|*)    invalidInput
                        ;;
            esac
        done
    else
        #if name does not exist in hosts, get ip addr
        getIpAddr
        if checkAddressExistsInHosts; then
            while true; do
                read -p "Add as an alias to \"$HOSTS\", or [q]uit? [y/n/q]  " REPLY
                case $REPLY in
                    [qQnN]) exitError "Aborted by User." 0
                            ;;
                    [yY])   break #addAliasToHosts
                            ;;
                   ""|*)    invalidInput
                            ;;
                esac
            done
        fi
    fi

    getConnectionType
    getPrinterType
    lpadd
    doAddPrinter
    acceptPrinter
}

##
##  MAIN PROGRAM
##

sanity

while true; do
    main
done
unset GREP_OPTIONS
\$\endgroup\$

3 Answers 3

2
\$\begingroup\$

To answer your question, yes, it can be improved, but that could be said about any piece of code, right? And perfection is in the eye of the observer, so I'll stick to the things that have proved gotchas for me, and those focus around conditional constructs.

As was mentioned in the comments on your original post on SO, [ ] is better than test. But we can go one step further. Some times you'll see scripts that have something similar to this.

if [ x"$VAR" = x"" ]; then ...; fi

This was to avoid a situation where an empty variable could cause a syntax error. Using [[ ]] automatically avoids this an allows you to write more cleanly.

if [[ $VAR == "" ]]; then ...; fi

This also allows you to have more natural logic operators in your conditional.

if [[ $VAR1 == "yep" && $VAR2 == "sure" ]]; then ...; fi

There are many other cool things that this buys you too. You can now do regular expression comparisons inline.

if [[ $IP =~ ^[0-9]+\.[0-9]+\.[0-9]+\.[0-9]+$ ]]; then echo "valid ip"; fi;

The last thing I'll leave you with the (( )). This is very very handy when you know you are operating on numbers. It allows you, among many other things, to compare numbers as numbers rather than strings (as with [ ] or [[ ]]).

if (( $ZERO == 0 )); then echo "yep, it's zero"; fi

I'll leave you to explore the recent version of the manual for more cool stuff! :)

\$\endgroup\$
5
  • \$\begingroup\$ Why is [] better than test? (The majority of projects that I've worked on prefer test to [] so I'm interested in your rationale.) \$\endgroup\$
    – CB Bailey
    Oct 12, 2011 at 20:02
  • \$\begingroup\$ @CharlesBailey, Functionally, [ ] is the same as test, so in that regard it doesn't matter. Many find [ ] is easier to read than test. But, and this is important, neither compare to the power of [[ ]] which should be regularly used in favor of both [ ] and test. Check out the documentation on conditional constructs for more info. (Notice that [ ] and test aren't even mentioned any more! They're deprecated!) \$\endgroup\$
    – perden
    Oct 12, 2011 at 20:24
  • 2
    \$\begingroup\$ [[ ... ]] isn't in the POSIX standard, though, so you immediately lose compatibility with standard Bourne shells. Can you provide a reference to test and [] being deprecated, I am very surprised to here that. \$\endgroup\$
    – CB Bailey
    Oct 12, 2011 at 20:29
  • \$\begingroup\$ @CharlesBailey, The latest version of the Bash manual doesn't reference to the use of [ ] and only details [[ ]] and since [[ ]] encompass the functionality of [ ], that's deprecation by definition! :) Also note: (1) [[ ]] was introduced near its present capacity circa Bash-2.0, but has since developed. (2) [[ ]] is not in POSIX, but the standard does mention that these characters are used as reserved words in some implementations (i.e. Bash). (3) The original question is about Bash, so it's worth mentioning [[ ]] as a good feature to use! :) \$\endgroup\$
    – perden
    Oct 12, 2011 at 21:03
  • 2
    \$\begingroup\$ test and [ are referenced here: gnu.org/s/bash/manual/bash.html#Shell-Builtin-Commands . They are not marked as deprecated and as bash "intended to be a conformant implementation of the ieee posix Shell and Tools portion of the ieee posix specification", nor should they be. I think you were misusing the word "deprecated". I always think it's good to warn about non-portable constructs even when they have useful advantages. \$\endgroup\$
    – CB Bailey
    Oct 12, 2011 at 21:11
1
\$\begingroup\$

I see only a couple of minor issues.


I suggest to flip the parameters of exitError: make the exit code the first and the message the second. As the message tends to get long, the exit code parameter is easy to overlook, which can lead to accidental misuses.


No need to quote literal values like '0' here, you can write simply 0:

if [ $EUID != '0' ]; then

Instead of declaring a "useless" variable here:

declare USELESS
printf "\nInvalid input.\n" >&2
read -p "Press Any Key To Continue..." USELESS
#unset USELESS
printf "\n"

You could just not declare anything at all:

printf "\nInvalid input.\n" >&2
read -p "Press Any Key To Continue..."
printf "\n"

The handling of printer name input is a bit odd:

    read -p "Enter printer name, or [q]uit:  " PRINTER_NAME
    case $PRINTER_NAME in
       [qQ])    exitError "Aborted by User" 0
                ;;
        ??*)    break
                ;;
       ""|*)    invalidInput        #blank line or anything else
                ;;

An invalid value is a blank or a single-letter name other than "q" or "Q". If it's important that the name should not be a single-letter, then it would be better to mention that in the prompt. If it's not so important, then the switch can be simplified a bit:

    read -p "Enter printer name, or [q]uit:  " PRINTER_NAME
    case $PRINTER_NAME in
       [qQ])    exitError "Aborted by User" 0
                ;;
         "")    invalidInput
                ;;
          *)    break
                ;;

This kind of case statement appears at many places:

     ""|*)  invalidInput

When * is one of the values, all other values are unnecessary.


Since the script uses /bin/bash, you could simplify >/dev/null 2>&1 as &>/dev/null.


I'm not a huge fan of flag variables. For example in doAddPrinter you have this:

declare SUCCESS

# ... (many many lines)

if checkPrinterExistsInSystem; then
    SUCCESS=0
    lpstat -v "$PRINTER_NAME"
    printf "Printer successfully added.\n\n"
else
    SUCCESS=1
    printf "Printer was not successfully added!\n\n" >&2
    #exit
fi
return $SUCCESS

I suggest to not declare SUCCESS at the top and return at the end, but to return in the branches of the final conditional:

if checkPrinterExistsInSystem; then
    lpstat -v "$PRINTER_NAME"
    printf "Printer successfully added.\n\n"
    return 0
else
    printf "Printer was not successfully added!\n\n" >&2
    return 1
fi

If you really want to use the SUCCESS variable, then at least declare it right before it's used, so the reader doesn't need to scroll up to verify the declare or local keywords.

\$\endgroup\$
0
\$\begingroup\$

Some other minor notes on portability (I know that from your shebang you are using bash but a portable script is usually better :-)

  • declare is not portable (you could use subshells ( ... ) or $( ... ) to define local variables

  • here-strings are not portable

\$\endgroup\$

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge that you have read and understand our privacy policy and code of conduct.

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