Skip to content

triangle interface - why take a list? #729

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
stevejb71 opened this issue Mar 19, 2017 · 14 comments
Open

triangle interface - why take a list? #729

stevejb71 opened this issue Mar 19, 2017 · 14 comments

Comments

@stevejb71
Copy link
Contributor

In the triangle exercise, the canonical data shows a list as the input to functions like equilateral.

Is part of the exercise to check that the list is of length 3? If so, there are no tests on the case of incorrectly sized lists.
If not - surely it would be better to have 3 number parameters as the inputs.

@ErikSchierboom
Copy link
Member

Well, as mentioned in #722, the Google JSON Style Guide usually recommends avoiding nesting structures in favor of flatter ones, but that is not a rule:

Data elements should be "flattened" in the JSON representation. Data should
not be arbitrarily grouped for convenience.

In some cases, such as a collection of properties that represents a single
structure, it may make sense to keep the structured hierarchy. These cases
should be carefully considered, and only used if it makes semantic sense.
For example, an address could be represented two ways, but the structured
way probably makes more sense for developers:
(...)

However, it is not stated explicitly that we try to adhere to this style guide, so maybe that is something that we should add?

@stevejb71
Copy link
Contributor Author

My initial implementation was guided by the canonical data, so my functions took a list. I'm suggesting that:

          "description": "true if all sides are equal",
          "property": "equilateral",
          "a": 2,
          "b": 2,
          "c": 2,
          "expected": true

is clearer than

          "description": "true if all sides are equal",
          "property": "equilateral",
          "sides": [2, 2, 2],
          "expected": true

@kotp
Copy link
Member

kotp commented Mar 19, 2017

I think the generator, in this case this means the person translating for the purpose of using that file, can interpret as it sees fit. In the Ruby generator, there is no arguments given to equilateral in the tests.

The only method that takes any arguments there is the constructor, and indeed they chose to take the list (as an array) for the argument that is accepted.

Which implementation, @stevejb71 on the exercism site or for creating a generator, or for creating the exercise example for a track?

I can see that the first example that is "clearer" makes some assumptions for naming the sides as well. It happens to be the same names that are used in the example Ruby exercise as well, which I am assuming comes from the canonical data as well. So even without the explicit mention of 'a, b, c' this did happen. So is the first example required to have this happen, or can this happen fairly naturally from your second example?

@rbasso
Copy link
Contributor

rbasso commented Mar 19, 2017

Is part of the exercise to check that the list is of length 3?

Certainly not!

...If not - surely it would be better to have 3 number parameters as the inputs.

I don't share your certainty. 😄

Using an array, we need to assume that it has length three. Using an object, we have to assume that is has just the keys for a, b and c.

Both solutions try to encode a triple, but - because JSON has no explicit support for tuples - there is no perfect choice here.

I think the array named sides is short, descriptive and readable. Using the three keys seems a little artificial to me, because a, b and c are just arbitrary names to three things that have exactly the same meaning.

That said, I agree it is a little annoying to use an array to represent a tuple.

@stevejb71
Copy link
Contributor Author

surely it would be better to have 3 number parameters as the inputs.

I meant to put a question mark after that :).

I still think there are downsides to using a list instead of separate json keys:

  • every other exercise I've seen (admittedly I've not looked at every json file) has one json key per parameter. This exercise is inconsistent in that regard.
  • a human implementing the exercise might be led to use a list for the triangle inputs, which is not as good as a tuple, or 3 separate parameters. (A comment might fix this.)
  • a code generator (for example the OCaml one which I've written) has to have extra code to convert the list to 3 separate parameters.

@jtigger
Copy link
Contributor

jtigger commented Mar 20, 2017

From a general design perspective, all other things being equal, if I have a piece of data of known shape, using a data structure of exactly that shape is more concrete (and by definition requires less inference).

Admittedly, in this case, the difference is slim.

A triangle will never have fewer or more than three sides... forever. Is that "permission" enough to express that "rule" concretely?

For those who want to iterate over the tuple, they could implement that interface/protocol/contract?

@petertseng
Copy link
Member

petertseng commented Apr 25, 2017

This will become relevant to other exercises, not just this exercise. As a result of #721 or #722, either queen-attack or robot-simulator could end up using arrays as their inputs. I agree that arrays simply an artifact of the fact that JSON has no tuples.

A similar decision, I imagine, had to be made in #755 which instead of using [1, 10] uses {"column": 1, "row": 10}

However, triangle also differs slightly from these above three exercises because triangle doesn't particularly care about the order of its inputs, whereas the other three do (rank vs file, row vs column). So the solution will seem a bit artificial because a/b/c or side1/side2/side3 are interchangeable names.

I do prefer side1/side2/side3 over a/b/c though.

a human implementing the exercise might be led to use a list for the triangle inputs

This is also a concern of mine. I agree that even though it's encoded in JSON as an array, generated tests shouldn't.

I don't particularly care which solution is taken:

  • Leave it as-is, but add a comment about the nature of the array.
  • Just make them "side1": 4, "side2": 5, "side3": 6

They're both better than what happens now.

@Insti
Copy link
Contributor

Insti commented Apr 25, 2017

I like "sides": [2, 2, 2]

It's already like this.
Naming your variables thing1,thing2,thing3 is a smell that indicates you should be using some way of grouping your data, like an array.
Triangle doesn't need to care about the order of its inputs.
There will always be exactly three elements in the array. (Enforced by PR reviews of changes to the .json file)
Tracks can split the array into 3 separate variables for their tests if they want. (Although the reverse is also true.)

@stevejb71
Copy link
Contributor Author

I think a comment, advising that the function should take 3 arguments rather than a list, would be sufficient.

@Insti
Copy link
Contributor

Insti commented Apr 26, 2017

I think a comment, advising that the function should take 3 arguments rather than a list, would be sufficient.

In some languages arguments are a list.

@petertseng
Copy link
Member

Good point. I think we're at the point where we can write a PR and examine how a proposed comment would or would not fit certain languages.

@hilary
Copy link
Contributor

hilary commented Apr 27, 2017

I thought that one of the principles of exercism was that we don't push folks towards a specific implementation.

@petertseng
Copy link
Member

don't push folks towards a specific implementation

Indeed, that is a subpart of the decision not to test intermediate functions (exercism/discussions#41).

There are two places where it's necessary for us to be careful:

  • The tests listed in the canonical data.
  • Each track's tests (resulting from the canonical data, if present).

Since this issue doesn't deal with the former, it's important to consider how the choice we make affects the latter.

I believe a comment is appropriate.

@rpottsoh
Copy link
Member

I think we're at the point where we can write a PR and examine how a proposed comment would or would not fit certain languages.

Was a PR ever opened? I have not been able to find any closed PRs that might to relate to this issue. Is this issue still worth pursuing?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

9 participants