Goodgame Empire coin collector with random offsets in (almost-POSIX) Bash

Question

Today, my goal was again to improve my POSIX shell scripting abilities.

In one MMO game called Goodgame Empire, there is possible to code yourself a coin (tax) collector. But that's of less importance than my intention above.

Sorry, I am not able to avoid arrays, well it could be done, but an elegant way? I will be happy to see any and all suggestions.

#!/bin/bash

print_usage_and_exit()
{
    echo "Usage: $0 [-1]"
    echo "      -1: One-time coin collect."
    echo "Default: Repeat coin collecting until CTRL+C is pressed."
    exit 1
}

no_repeat=false

while getopts ":1h" option
do
    case "${option}" in
        1)
          no_repeat=true
          ;;
        h | *)
          print_usage_and_exit
          ;;
    esac
done

shift $((OPTIND - 1))

# ------------------------------------------------------------------------------

# global constants and variables - self-explanatory

declare -r window_name_chrome="Goodgame Empire - Google Chrome"

declare -r screen_resolution=$(xdpyinfo | awk '/dimensions:/ {print $2}')

# we need to keep track of these two variables used by mouse_click function
previous_rand=10
operation_add=true

# ------------------------------------------------------------------------------

print_error_and_exit()
{
    # check if exactly two arguments have been passed
    test "$#" -eq 2 || print_error_and_exit 2 "print_error_and_exit(): There have not been passed exactly two arguments!"

    # check if the first argument is a number
    is_number "$1" || print_error_and_exit 3 "print_error_and_exit(): The argument #1 is not a number!"

    bold=$(tput bold)
    red=$(tput setaf 1)
    nocolor=$(tput sgr0)

    echo "$bold$red$2 Exit code = $1.$nocolor" >&2
    exit "$1"
}

# ------------------------------------------------------------------------------

is_number()
{
    # check if exactly one argument has been passed
    test "$#" -eq 1 || print_error_and_exit 4 "is_number(): There has not been passed exactly one argument!"

    # check if the argument is an integer
    test "$1" -eq "$1" 2>/dev/null
}

# ------------------------------------------------------------------------------

random_number()
{
    # check if exactly two arguments have been passed
    test "$#" -eq 2 || print_error_and_exit 5 "random_number(): There have not been passed exactly two arguments!"

    # check if the arguments are both numbers
    is_number "$1" || print_error_and_exit 6 "random_number(): The argument #1 is not a number!"
    is_number "$2" || print_error_and_exit 7 "random_number(): The argument #2 is not a number!"

    # generate one pseudo-random integer within the specified range
    shuf -i "$1-$2" -n 1
}

# ------------------------------------------------------------------------------

activate_window_via_name() {
    # check if exactly one argument has been passed
    test "$#" -eq 1 || print_error_and_exit 8 "activate_window_via_name(): There has not been passed exactly one argument!"

    xdotool search --name "$1" windowactivate --sync
}

# ------------------------------------------------------------------------------

maximize_active_window() {
    # check if no argument has been passed
    test "$#" -eq 0 || print_error_and_exit 9 "maximize_active_window(): There has been passed some argument, none expected!"

    wmctrl -r :ACTIVE: -b add,maximized_vert,maximized_horz
}

# ------------------------------------------------------------------------------

mouse_click() {
    # check if exactly two arguments have been passed
    test "$#" -eq 2 || print_error_and_exit 10 "mouse_click(): There have not been passed exactly two arguments!"

    # check if both of the arguments are numbers
    is_number "$1" || print_error_and_exit 11 "mouse_click(): The argument #1 is not a number!"
    is_number "$2" || print_error_and_exit 12 "mouse_click(): The argument #2 is not a number!"

    # 1. invert the operation_add boolean value,
    #    it seems Bash does not have inbuilt command for that
    # N: operation_add determines whether we will be adding or
    #    subtracting the random number later

    test "$operation_add" = true && operation_add=false || operation_add=true

    # 2. generate pseuso-random integer between 0 and 7, inclusive,
    #    if the generated number is the same as the previous_rand,
    #    generate until it is different
    # N: rand will be later used as pixel offset from the given coordinates

    # we define a constant for randomness

    declare -r randomness=7

    rand=$(random_number 0 "$randomness")

    while [ "$rand" -eq "$previous_rand" ]
    do
        rand=$(random_number 0 "$randomness")
    done

    # 3. we don't want to repeat clicks right with the same offset,
    #    so we store information about the previous_rand here

    previous_rand="$rand"

    # 4. depending on the boolean value of operation_add,
    #    we either add the rand, or subtract it to/from the position x/y

    if [ "$operation_add" = true ]
    then
        pos_x=$(($1 + rand))
        pos_y=$(($2 + rand))
    else
        pos_x=$(($1 - rand))
        pos_y=$(($2 - rand))
    fi

    #  activate Goodgame Empire window and wait for sync,
    #  we need to do this before each click,
    #  because the user may have clicked on some other window
    #  during the 2 second delay
    activate_window_via_name "$window_name_chrome"

    maximize_active_window

    # xdotool can move mouse and simulate button clicks and more
    # ----------------------------------------------------------
        # move the mouse cursor to the given position and wait for sync
        # click the left mouse button
        # restore the original mouse cursor position and wait for sync
        # wait for 2 seconds
    xdotool \
        mousemove --sync "$pos_x" "$pos_y" \
        click 1 \
        mousemove --sync restore \
        sleep 2
}

# ------------------------------------------------------------------------------

mouse_click_coords() {
    # accept all parameters together as one array
    local coords=("$@")

    # self-explanatory, but non-memorizable
    array_items_count="${#coords[*]}"

    # check if there have been passed exactly ten arguments
    test "$array_items_count" -eq 10 || print_error_and_exit 13 "mouse_click_coords(): There have not been passed exactly ten arguments!"

    for (( i = 0; i < "$array_items_count"; i += 2 ))
    do
        mouse_click "${coords[$i]}" "${coords[$i + 1]}"
    done
}

# ------------------------------------------------------------------------------

collect_coins_1920x1080() {
    local coords=(
        1895 955
        1104 691
        1131 660
        1145 570
        1199 381
    )

    mouse_click_coords "${coords[@]}"
}


# ------------------------------------------------------------------------------

collect_coins_3840x1080() {
    local coords=(
        3815 955
        3024 691
        3051 660
        3065 570
        3119 381
    )

    mouse_click_coords "${coords[@]}"
}

# ------------------------------------------------------------------------------

collect_coins() {
    case "$screen_resolution" in
        1920x1080) collect_coins_1920x1080
        ;;
        3840x1080) collect_coins_3840x1080
        ;;
    esac
}

# ------------------------------------------------------------------------------

if [ "$no_repeat" = false ]
then
    echo "Repeating coin collecting until CTRL+C is pressed!"

    while true
    do
        collect_coins

        # wait for 10 minutes
        sleep 600
    done
else
    echo "One-time coin collecting!"

    collect_coins
fi

Answer 1

You already have a great review; here's a supplement.

Consider setting errexit and nounset flags

You can make the script more robust with

set -e -u

Both options are POSIX compliant.

Distinguish between asked-for and unasked-for usage

This is just a nicety, but when I ask a program for its help with -h or --help, I expect it to print on standard output (so I can pipe to a pager if it's long, or to a printer, etc.) and exit with a success status. If I enter a unrecognised option, then I expect error output and a failure exit code.

I suggest (with the obvious change to make print_usage):

case "${option}" in
    1)
        no_repeat=true
        ;;
    h)
        print_usage
        exit 0
        ;;
    *)
        print_usage >&2
        exit 1
        ;;
esac

As a really tiny point, in the usage message, it's slightly presumptuous to assume that the terminal maps Ctrl+C to the interrupt signal - perhaps the message could just say "until interrupted" or "indefinitely" or similar.

Account for all cases

There's another case statement here:

collect_coins() {
    case "$screen_resolution" in
        1920x1080) collect_coins_1920x1080
        ;;
        3840x1080) collect_coins_3840x1080
        ;;
    esac
}

What happens if $screen_resolution doesn't match either of those two values? No action, and no warning message. We could add a catch-all *) to at least do something in that condition.

A more radical alternative is to do away with the case entirely, and compose the name of the function to be called:

collect_coins() {
    if ! "collect_coins_$screen_resolution"
    then echo "Unrecognised screen resolution" >&2; exit 1
    fi
}

For extra robustness, use the type builtin to confirm that collect_coins_$screen_resolution is a function (and not a command on $PATH) before calling it.

Is screen size really a constant?

We only check the screen dimensions once:

declare -r screen_resolution=$(xdpyinfo | awk '/^  dimensions:/ {print $2}')

However, for a long-running program, this may well be untrue. Many X servers can change their real or virtual output size in that timescale; a user might even target a different monitor (such as on a laptop which might be docked to a larger screen).

Note also that xdpyinfo will print a dimensions line for each screen of the display - you might need to be more selective.

Does mouse_click_coords require exactly 5 positions?

It's not obvious to me why this function needs five pairs of arguments. It can be made to process pairs of arguments until we run out:

mouse_click_coords() {
    while [ $# -gt 1 ]
    do
        mouse_click "$1" "$2"
        shift 2
    done

    if [ $# -eq 1 ]
    then
        print_error_and_exit 13 "mouse_click_coords(): coordinates must be in pairs"
    fi
}

And the collect_coins_* functions don't need array variables:

collect_coins_1920x1080() {
    mouse_click_coords \
        1895 955 \
        1104 691 \
        1131 660 \
        1145 570 \
        1199 381
}

collect_coins_3840x1080() {
    mouse_click_coords \
        3815 955 \
        3024 691 \
        3051 660 \
        3065 570 \
        3119 381
}

Answer 2

Requirements

The script uses many non-standard programs to do its job (shuf, xdotool, ...). I would add a check at the beginning to verify that all dependencies are present in the system. Without that, if something is missing, the script may appear to work, or partially work, which could be more frustrating than a message upfront about all requirements.

Managing error codes

It seems you try to use a different error code for each different failure point. The error codes are hardcoded at each error location, and this way it's hard to tell if the values are really unique. You could give the different codes names, and define them at the top of the file, where it's easy to see.

Do you really need to distinguish the different exit conditions? Do you have other code that relies on these values? I'm not sure this is worth the trouble.

Error reporting

I find these sentences very strange English:

test "$#" -eq 2 || print_error_and_exit 5 "random_number(): There have not been passed exactly two arguments!"

And it would help debugging to include a bit more information about the invalid arguments, for example:

test "$#" -eq 2 || print_error_and_exit 5 "random_number(): Expected 2 arguments, got $#: $@"

Style

There is a mix of function declaration styles in the script:

random_number()
{
...
}

activate_window_via_name() {
...
}

It's good to use one style consistently. I prefer the second one.

Arithmetic context

This could be written simpler:

for (( i = 0; i < "$array_items_count"; i += 2 ))

Like this:

for (( i = 0; i < array_items_count; i += 2 ))

Alternative to using arrays

Sorry, I am not able to avoid arrays, well it could be done, but an elegant way? I will be happy to see any and all suggestions.

Ok, so you want to avoid arrays because they are unspecified in POSIX. A possible alternative could be to use a simple whitespace separated value instead. For example, instead of this:

local coords=(
    1895 955
    1104 691
    1131 660
    1145 570
    1199 381
)

You could do something like this:

local coords="1895x955 1104x691 1131x660 1145x570 1199x381"

That is, add a non-white-space separator in the coordinate pairs to avoid word-splitting. Now you will be able to loop over the coordinates with a simple for coord in $coords, and then use parameter expansion syntax (x=${coord%%x*}; y=${coord##*x}) to split the value. It's not very elegant, but without arrays, I think it's the best you can get.