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

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).

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!

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 }