If statement best practice

by safesploit   Last Updated August 13, 2018 20:05 PM

I am reviewing best practice with if statements. Below in example A I include the entire code to be run and ensure it remains within the if statement. While for example B the code to be executed resides outside the if statement.

From my understanding of Best practice on if/return, example A appears to be best practice.

Example A

#!/bin/bash
if [ `whoami` != 'root' ]
then
  echo "MUST be ROOT!"
  exit
else
  ifconfig -a | sed 's/[ \t].*//;/^\(lo:\|\)$/d'
  read -p "Enter interface: " int
  mac=$(macchanger $int | grep 'Permanent MAC:' |  grep -o -E '([[:xdigit:]]{1,2}:){5}[[:xdigit:]]{1,2}')
  ifconfig $int down
  iwconfig $int mode manage
  macchanger $int $mac
  ifconfig $int up
  echo "$int in stock configuration with MAC: $mac"
  echo "Connect to a Wi-Fi network and run 'ping 1.1.1.1'"
  sleep 2 && cinnamon-settings network &
fi

Example B

#!/bin/bash
if [ `whoami` != 'root' ]
then
  echo "MUST be ROOT!"
  exit
fi
ifconfig -a | sed 's/[ \t].*//;/^\(lo:\|\)$/d'
read -p "Enter interface: " int
mac=$(macchanger $int | grep 'Permanent MAC:' |  grep -o -E '([[:xdigit:]]{1,2}:){5}[[:xdigit:]]{1,2}')
ifconfig $int down
iwconfig $int mode manage
macchanger $int $mac
ifconfig $int up
echo "$int in stock configuration with MAC: $mac"
echo "Connect to a Wi-Fi network and run 'ping 1.1.1.1'"
sleep 2 && cinnamon-settings network &

I can foresee a potential example C, which would include structure:

if [ `whoami` != 'root' ]
then
  echo "MUST be ROOT!"
  exit
if [whoami' == 'root' ]
  then
   ...
fi

However, this seems like creating an extra unnecessary condition, which uses additional resources. So, I will disregard example C.



Answers 1


In this particular case, I don't think there is much to recommend approach A versus approach B, or vice versa.

However, I disagree that in general it's better to use

if A then 
  stuff
else 
  other stuff
endif

versus:

if A then 
  stuff;
  exit/return;
endif

other stuff;

The problem with the nesting approach, is that when applied rigorously, you can get alot of nesting, and nesting is hard for humans to understand.

IMHO, the following is often simpler and clearer:

  if A then
    check any boundary cases that dont have mcuh to do the the core logic.
    either throw, or return or whatever makes sense.
  end;

  ASSERT (!A); /// and any further assertions of stuff you checked above.

  // then freely write code ignoring those corner cases as already handled.
Lewis Pringle
Lewis Pringle
August 13, 2018 19:39 PM

Related Questions




Break on default case in switch

Updated December 18, 2016 08:02 AM

Extensible way to create bash program

Updated May 11, 2017 08:05 AM