Finding legal knight moves in a chessboard in APL (follow-up)

Question

This question is a follow-up to this previous question of mine. _{Assuming I understood correctly what is outlined in this meta post.}

I wrote (and now, re-wrote) a function that takes as input a vector with two integers between 1 and 8 representing a position in a chessboard. This function should output a vector where each cell is a similar vector of integers, with the positions that a knight in the input position could reach.

E.g. for the input 1 1, my function should output (2 3)(3 2).

KnightMovesRevised ← {
  ⍝ Monadic function, expects a vector with 2 integers, e.g. (1 1)
  ⍝ Given a chessboard position, find the legal knight moves.
  ⍝ Returns vector of 2-integer vectors, e.g. (2 3)(3 2)

  ⍝ aux train to check if position is inside chessboard
  isInsideBoard ← ∧/(1∘≤∧≤∘8)

  signs ← ¯1 1 ∘., ¯1 1
  offsets ← (1 2)(2 1)
  moves ← , signs ∘.× offsets
  ⍝ list all the locations the knight could go to
  locations ← moves + ⊂⍵
  ⍝ and keep the valid ones
  valid ← isInsideBoard¨ locations
  valid/locations
}

Changes

From the previous version to this one, I

• Reformatted code a bit with a suggested naming convention, by naming an auxiliary train and adding a couple more comments;
• Removed the train used to write offsets, which I had used just to give a go at tacit programming. This is such a small vector that I think it makes more sense to hardcode it;
• Rewrote signs by writing twice the ¯1 1 and removing ,, ⍨. This made it slightly easier to digest and not much more annoying to type;

These changes were motivated by the two great reviews I got (here and here) and I was hoping I could get reviews on those changes, because I tried to adhere to their suggestions but I didn't necessarily agree with all of them.

Questions

(paired with the above)

• Are the extra comments ok or are they too much?
• Are signs and offsets defined in an acceptable way? I like the trade-off between hardcoding too much and using too many functions just to create a couple of constants.
• What is the standard spacing notation around ¨? Should I write f¨ arg, f ¨ arg, f ¨arg or f¨arg?

Answer 1

On your previous version you commented, "This works and gives the expected result for a series of test cases." But you never provided those test cases, right? I think the biggest thing missing here is test cases. Especially since test cases would quickly clarify the expected behavior of the function on weird inputs, and then you could maybe even get rid of some of the vague comments like

expects a vector with 2 integers, e.g. (1 1)

• I infer that the two integers are supposed to be in the range 1..8 (not 0..7 as one might expect if one'd been doing too much programming lately and not enough chess). What happens when they're not in the range 1..8?

• What happens when there are three integers in the vector, or one, or none?

• What happens when there's something other than integers in the vector?

I know you split out isInsideBoard into its own named function thanks to a comment on the earlier question; but if it's only ever used once, is that buying you anything? Honestly, as not-really-an-APLer-myself, ∧/(1∘≤∧≤∘8) is pretty much the only part of that code that I could instantly understand!

If I understand correctly, the output of KnightMovesRevised is a vector each of whose elements is suitable for feeding back into KnightMovesRevised; is that right? If so, that's good! You could even write a test case demonstrating how to find the number of cells that are exactly 2 knight-moves away from (1,1).

Answer 2

Answers to OP's questions

Are the extra comments ok or are they too much?

Looks good to me overall, though some of them contain redundant information (that is already explained as variable names):

  ⍝ aux train to check if position is inside chessboard
  isInsideBoard ← ∧/(1∘≤∧≤∘8)

Compare it with, e.g.

  ⍝ checks if position is inside chessboard, i.e. 1 ≤ both coords ≤ 8
  isInsideBoard ← ∧/(1∘≤∧≤∘8)

Or you could omit it entirely since the code is talking the intent very well by itself, and just name the function better:

  IsInsideChessBoard ← ∧/(1∘≤∧≤∘8)

which, by following the naming convention (function names capitalized), is made even more clear.

Are signs and offsets defined in an acceptable way? I like the trade-off between hardcoding too much and using too many functions just to create a couple of constants.

Also fine to me. I especially like how you decided to simply go with (1 2)(2 1) for offsets.

Nitpicking: Having an intermediate array of rank 3 or higher can make code hard to understand. In your current code, signs is a matrix and offsets is a vector, so signs ∘.× offsets yields a cube (rank 3 array). I'd suggest adding a , to signs:

  signs ← , ¯1 1 ∘., ¯1 1

What is the standard spacing notation around ¨? Should I write f¨ arg, f ¨ arg, f ¨arg or f¨arg?

There's no such thing in APL, partially because some APL editors strip away all spaces not relevant to tokenization. But considering that ¨ binds to the function on its left to modify its behavior, I believe f¨ arg is the most reasonable spacing.

---

Writing test cases

Expanding on Quuxplusone's suggestion.

Unfortunately, APL doesn't yet have a standard way to write unit tests. Yet we can find some examples of writing simple assertions. One striking example is from Roger Hui's Dyalog blog post, written back in 2015:

assert←{⍺←'assertion failure' ⋄ 0∊⍵:⍺ ⎕SIGNAL 8 ⋄ shy←0}

pcheck←{
  assert 2=⍴⍴⍵:
  assert (⍴⍵)≡(!⍺),⍺:
  …
  1
}

This cleverly uses dfns' guards to neatly list all the assertions to satisfy. If you run this in the interpreter and some assertion fails, a ⎕SIGNAL 8 is raised and execution is stopped at the line containing the failed assertion.

In Advent of APL, I use slightly different formulation to allow testing for multiple functions implementing the same thing (modified to meet the naming convention you're using):

Assert←{
    0=⍵:'Assertion Failure'⎕SIGNAL 11
    0
}
_Test←{
    F←⍺⍺
    Assert 0≡F'(())':
    Assert 0≡F'()()':
    Assert 3≡F'(((':
    Assert 3≡F'(()(()(':
    Assert 3≡F'))(((((':
    'All tests passed'
}
⍝ Actual testing
Solution _Test ⍬

You can try writing tests for your function in this style. Since the order of the output shouldn't matter, you could write something like this:

Sort←(⍋⊃¨⊂)
UnorderedEq←{(Sort ⍺)≡Sort ⍵}
Assert←{⍺←'assertion failure' ⋄ 0∊⍵:⍺ ⎕SIGNAL 8 ⋄ shy←0}
Test←{
  Assert (2 3)(3 2) UnorderedEq KnightMovesRevised 1 1:
  Assert (1 1)(1 5)(3 1)(3 5)(4 2)(4 4) UnorderedEq KnightMovesRevised 2 3:
  Assert 8 = ≢ KnightMovesRevised 3 5:
  'All tests passed'
}
⎕←Test ⍬

Try it online!

Answer 3

Redundant parenthesis

isInsideBoard ← ∧/(1∘≤∧≤∘8) was converted from inline explicit code. Back then, the train 1∘≤∧≤∘8 needed parenthesising. However, now that you've broken out this code to a separate tacit function, the ∧/ actually forms an atop (a 2-train) with the existing train, and since the original train was a fork (has 3 parts), it can simply be a 4th:

isInsideBoard ← ∧/ 1∘≤∧≤∘8

Shorter name

Bubbler suggested renaming this function to IsInsideChessBoard. However, I often find that a function that determines or computes something that can be given a good name (valid in this case) can often have a matching function name (that'd be Valid). I think it is obvious from context of the containing function that validity is defined to be "inside the chess board". Alternatively, you could name the function and variable Inside and inside.

Structure your code

I can't remember a thing, so I'd prefer defining the helper function as close as possible to where it is first used. I'd space it from the preceding code as two sections; the first finding all locations and the second determining their validity. Each section can appropriately begin with a comment on what it does. Maybe even exdent the comments to further emphasise it?

In summary

With these three changes:

KnightMovesRevised ← {
  ⍝ Monadic function, expects a vector with 2 integers, e.g. (1 1)
  ⍝ Given a chessboard position, find the legal knight moves.
  ⍝ Returns vector of 2-integer vectors, e.g. (2 3)(3 2)

 ⍝ list all the locations the knight could go to
  signs ← ¯1 1 ∘., ¯1 1
  offsets ← (1 2)(2 1)
  moves ← , signs ∘.× offsets
  locations ← moves + ⊂⍵

 ⍝ and keep the valid ones
  Inside ← ∧/ 1∘≤∧≤∘8
  inside ← Inside¨ locations
  inside/locations
}

Answer 4

Having taken into account the feedback I got from the three answers that were posted, plus using my own brain, I think a good revision of the code in the question entails:

1. ensuring signs is a vector instead of a matrix by using , right before assigning;

2. moving the definition of the function isInsideBoard closer to where it is used;

3. renaming the function isInsideBoard to IsInside and rename the corresponding variable to inside;

4. removing unnecessary parentheses in the IsInside function but keeping a space to separate the final ∧/ from the fork 1∘≤∧≤∘8;

All in all, the code ends up looking like this:

KnightMovesRevised ← {
  ⍝ Monadic function, expects a vector with 2 integers, e.g. (1 1)
  ⍝ Given a chess board position, find the legal knight moves.
  ⍝ Returns vector of 2-integer vectors, e.g. (2 3)(3 2)

  ⍝ List all the locations the knight could go to
  signs ← , ¯1 1 ∘., ¯1 1
  offsets ← (1 2)(2 1)
  moves ← , signs ∘.× offsets
  locations ← moves + ⊂⍵

  ⍝ Find which ones are inside the chess board
  IsInside ← ∧/ 1∘≤∧≤∘8
  inside ← IsInside¨ locations
  inside/locations
}