Passing a tuple to new in Queen attack exercise feels weird

[ijanos]

Passing a pair to a new function feels unidiomatic.

There is a discussion started on the site:
http://exercism.io/submissions/05c66333b1b44c12a224e8fe4884a50d

We can continue here.

[petertseng]

For me personally I think y'all have convinced me. Passing two tuples to a function might make sense to show the more logical grouping, but a single tuple seems an unnecessary extra level of grouping (and I still believe that despite the fact that you can destructure in the argument list)

[IanWhitney]

Here was my thought process when making that a tuple.

1. A co-ordinate on the board is the same as a point on a grid.
2. A point on a grid is the canonical example of a data clump
3. You get rid of data clumps by encapsulating them.
4. The simplest way to do that in this exercise was with a tuple.

I don't quite follow the argument that passing in a single tuple is odd. This could be an idiomatic thing in Rust (or systems languages) that I'm unfamiliar with. I feel the same way about passing tuples into new. Seems fine to me, but there may be an idiomatic reason to not do so.

Also, from a design perspective, one of the more interesting things about the current Queen Attack is what you do with the tuple once you pass it in. Yes, you can just use it as a tuple, or you can encapsulate it, name it and give it behavior

[petertseng]

To me it sounds like there are two points of view that make sense based on the context.

---

In isolation, one may see the tuple as an unneeded extra level of grouping. If all the Queen tests are just testing queens with certain ranks and files that are sourced form somewhere we need not care about, why should it be that they values passed to us get packed up in a tuple? Especially if I'm just going to have to unpack the tuple immediately to operate on its two members anyway?

---

On the other hand, as part of a larger system that may pass coordinate points around, tuples make sense if tuples are the representation of the coordinate points (and they indeed are a very reasonable representation).

If I had a list of coordinates that I wanted to turn into queens, it's likely that I would want to have them as a list of (i8, i8), rather than two lists of i8.

And only if Queen::new takes a single (i8, i8) can you do something like Queen::new(give_me_some_coordinates()) (obviously give_me_some_coordinates() would be some function that gives you an (i8, i8)). I currently do not know of a way to make this work if Queen::new was (rank: i8, file: i8), and my attempts on the playground were unsuccessful; I don't think Rust has a splat operator that will splat the tuple in (making the commented line work).

[petertseng]

Another potential thought: Maybe I didn't want to internally represent a position as a tuple! Notice that earlier I said (with emphasis added):

tuples make sense if tuples are the representation of the coordinate points

What if the conditional in that sentence were false? What if I wanted to represent my position as a struct with named members instead of a tuple? I'm out of luck, it has to be a tuple since that's what Queen::new expects.

Of course I can change the representation I use internally, but the representation I have to accept from external callers is constrained by the test suite. That's perhaps not too bad (you often have to be liberal in what you accept), but if it is too bad, then here is another idea:

If the exercise would separate out the "Create and validate a position given a rank and file" versus "Given a queen at a certain position, can it attack a queen at this other position", we could envision a world where students are free to choose whatever internal representation of position makes sense, whether that be tuple or struct or anything else! (a single int with the value of rank*8 + file?!)

My memory is getting a bit fuzzy, but I think that was one of the iterations considered when Queen Attack was implemented for Rust but there was a concern about - how do we order the tests in a good way to allow the student to get to the first passing test quickly and iterate quickly from there?

I'll review the discussion around the initial implementation and see if I get any ideas.

E: Hmm, actually it was just that the final iteration we landed on is not too far from allowing this suggestion to work, since our first tests are fundamentally about testing valid and invalid positions. So we could consider that (Note: the tests would get a bit more verbose)

[kytrinyx]

we could envision a world where students are free to choose whatever internal representation of position makes sense

That's the winning argument as far as I'm concerned. (Of course, I wouldn't be surprised if you had an argument tomorrow that convinced me of the opposite, but still. This is a good one.)

the tests would get a bit more verbose

I'm personally OK with that, if it allows more freedom at the level of the implementation.

[IanWhitney]

we could envision a world where students are free to choose whatever internal representation of position makes sense

That's the winning argument as far as I'm concerned. (Of course, I wouldn't be surprised if you had an argument tomorrow that convinced me of the opposite, but still. This is a good one.)

I'm not sure I follow. How would this look? Since the tests would include functions with specific signatures, I don't see how students could choose their own approach without also changing the test suite.

Like, if we stick with tests containing the code Queen::new((1,2)) then students can't write their new implementation to accept rank and file parameters, right?

Edit:

Ok, after re-reading, I think I might follow.

fn test_coordinate_constructor() {
  let rank = 1;
  let file = 2;
  let coordinate = ChessCoord::new(rank, file)
  assert_eq!(coordinate.rank(), 1);
  assert_eq!(coordinate.file(), 2);
}

//test here for invalid chess coordinates

fn test_queen_construction() {
  let queen = Queen::new(ChessCoord::new(1,2))
}

Is that the proposal? Because it would let students store their representation of a coordinate in any way they choose?

[petertseng]

Yeah, you've essentially got it. Note that, whereas currently Queen::new returns a Result, I would think ChessCoord (which I might bikeshed as Position, but eh) return a Result. Queen::new would take an unconditional ChessCoord and unconditionally return a Queen, so the assumption is ChessCoords can only hold valid chess coordinates.

[IanWhitney]

Coord is a terrible name, I just threw it there for an example. I can put together a sample of this and we can see if we like it.

[IanWhitney]

@petertseng When you say that Queen::new takes an unconditional Position, I'm not sure exactly what you mean. Do you think the tests will look like:

fn test_queen_creation() {
  let position = ChessPosition::new(1,2);
  let queen = Queen::new(position.unwrap());
  //etc.
}

[petertseng]

When you say that Queen::new takes an unconditional Position, I'm not sure exactly what you mean. Do you think the tests will look like:

Yup, this tells me I should accompany my suggestions with code (I would have were I not otherwise occupied), but you do have the right of it.

[IanWhitney]

What do you think, @ijanos?

[petertseng]

I am usually cautious about speaking for others, but notice the thumbs up on #118 (comment) and who it comes from

[ijanos]

Sorry for the late response, I'm from Europe and I guess that is responsible for the longer delays between messages, you are sleeping while I'm awake an vice versa :)

@IanWhitney I like your proposed code, I think it is a better API for this exercise.