3
\$\begingroup\$

This is the first time I've attempted writing something in bash, and would appreciate any feedback or criticism to make it better.

#! /bin/bash
macArr=( 0 0 0 0 0 0 )
for ((i=0; i < ${#macArr[@]}; ++i))
do
    let "byte=$RANDOM%255"
    # the first byte must have the least significant bits set to "10" 
    # in order to identify as a locally administered unicast MAC
    if [ $i -eq 0 ];then
        let "byte=(((byte>>1)|1)<<1)"
    fi
    prependZero=$([ $byte -lt 16 ] && echo 1 || echo 0)
    # convert byte from decimal int to hex string
    byte=$(printf "%x" "$byte")
    # uppercase any letters in the string
    macArr[$i]="${byte^^}"
    if [ $prependZero -eq 1 ];then
        macArr[$i]="0${macArr[$i]}"
    fi
done
printf "%s" "${macArr[@]}"
\$\endgroup\$
  • 1
    \$\begingroup\$ Instead of $RANDOM%255, shouldn't that be $RANDOM%256 ? \$\endgroup\$ – janos Feb 10 '16 at 21:52
4
\$\begingroup\$

Interesting idea, and pretty good first Bash script.

Bash arithmetics

Instead of this:

let "byte=$RANDOM%255"

I find this more readable:

((byte = RANDOM % 255))

You can use math like this within ((...)), without $ of variables, and with spaces around operators to keep the expressions nicely readable.

You can use this in conditions too, in fact, instead of this:

if [ $i -eq 0 ];then

It's better to write like this:

if (( i == 0 )); then

And here, this is really awkward, as you are running a sub-shell, and storing the output of echo commands:

prependZero=$([ $byte -lt 16 ] && echo 1 || echo 0)

When it could be simply:

((prependZero = byte < 16))

Padding with 0

Since you already use printf to convert from integer to hexadecimal, you can actually use it to 0-pad too:

byte=$(printf "%02x" "$byte")

This will pad with zeros on the left to make it 2-digits wide

Suggested implementation

With the above tips, the script can be simpler and cleaner.

#!/bin/bash

macArr=( 0 0 0 0 0 0 )
for ((i = 0; i < ${#macArr[@]}; ++i))
do
    ((byte = RANDOM % 255))

    # the first byte must have the least significant bits set to "10" 
    # in order to identify as a locally administered unicast MAC
    if (( i == 0 )); then
        ((byte = ((byte >> 1) | 1) << 1))
    fi

    # convert byte from decimal int to hex string
    byte=$(printf "%02x" "$byte")

    # uppercase any letters in the string
    macArr[$i]="${byte^^}"
done
printf "%s" "${macArr[@]}"
\$\endgroup\$
  • 1
    \$\begingroup\$ awesome advice, thank you very much. i'm learning a lot today. \$\endgroup\$ – Mr Griever Feb 10 '16 at 22:54

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

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