The intent here is that an engineer who is not a Bash expert
should be able to readily maintain the code some months down the road.
I do not know much about bash
You might be that future maintenance engineer.
Be kind to your future self.
And thank you for that nice shebang.
true
get_placement() {
while :
Please don't do that.
The Bourne shell has a long and sordid history
which involves various (almost) work-alikes.
The special builtin :
colon operator used
to be important when writing scripts.
Nowadays, please just write while true
.
It's not like we're going to fork off a /usr/bin/true
child.
The shebang explained that we're targeting a shell, bash
,
which according to $ type true
offers true as a shell builtin.
Using arcane punctuation makes it harder for engineers
to google for relevant technical advice.
Using words (true
) lets polyglot engineers
leverage their knowledge from other environments
into this one.
shellcheck
Code review submissions should lint clean before being submitted.
When writing shell code it is easy to produce a result,
but it is a minor challenge to make the script correct,
due to quoting concerns and other minutiae.
Lint this code, and follow the linter's advice, so it lints clean.
'Nuff said.
spurious default
I don't understand this line:
draw_board() {
...
echo "${1:- }|${2:- }|${3:- }"
That is, why are we defaulting to SPACE?
It seems that only [XO-]
will ever be passed in.
I am reluctant to support defaulting which is never used,
out of concern that it may "paper over" some code defect
that may be injected in future.
char vs int; magic constant
digit_pattern="^[0-$((BOARD_SIZE-1))]\$"
One can play tic-tac-toe on boards of various sizes,
including 2×2, 3×3, 4×4, ....
The BOARD_SIZE constant is lovely and I thank you for it.
Here, the digit_pattern
expression is tightly coupled
to the ... = 9
assignment.
It precludes using a larger board, as then we'd need
to worry about multi-digit labels for the squares.
The key difficulty is that we're validating against
string (really char) values, instead of against integer values.
I am sad that we don't exit
with an error code
if this line encounters a "large" board size.
zero- vs one- origin
The review context showed a board with squares labeled 0 .. 8,
which is very helpful.
I'm surprised the game never displays that to the player.
Consider using 1 .. 9 instead,
even if that means allocating storage for an unused zeroth square.
Then interpreting a bash expression like $8
would be
more straightforward, it would have a more obvious
correspondence to a certain square.
side effect comments
# sets $ended to whether the game has ended and $winner to the winner
get_winner() {
Thank you for explaining a non-obvious part of the
contract,
it's very helpful.
This function is Too Long, as one cannot read all of it at once,
it needs vertical scrolling to review it.
You could e.g. easily Extract Helper
by writing a check_for_winner
function.
On which topic, the check is entirely too tedious.
Better to (roughly) loop over 159 357 123 147
.
Also, the vertical check is incorrect, leading to prompts like this:
-|O|X
-|O|X
-|O|X
Please enter a valid move (0-8)
The shift 3
doesn't interact with the column check as you wanted it to.
The row check works fine.
test suite
There isn't one, and you clearly need it,
given the column check code defect.
Write down some example move sequences, and the expected
winner outcome and board state.
Automating this will allow you to view Green bar before each commit.
UX
I found it a bit surprising that the prompt doesn't mention
whether it's currently the turn of O
or X
.
It's not necessarily obvious,
since an invalid input of e.g. 9
keeps current player the same.
I was surprised that CTRL/D didn't exit the game due to EOF.
An interactive player can always resort to CTRL/C.
But automated unit tests may want to use redirection
from an example game transcript file,
and so reliably terminating on EOF might become an important requirement.
This code only achieves a subset of its design goals.
I would not be willing to delegate or accept maintenance tasks on it
in its current form.