pascals-triangle: Added test prototypes and more README info. #963
Conversation
|
@hekrause I haven't looked this all over, but it appears that you have an extra file in the commit that doesn't belong. |
|
@rpottsoh You are right! Some left overs from another issue. :-) |
There was a problem hiding this comment.
I said in exercism/rust#274 (comment) in April that a row >= 5 is the first row where 1 is not one of the addends. That could be a reason to add it.
I have no arguments in favour of additional cases beyond that.
As written, it doesn't seem like good ordering to have -1 between 4 and 5. I would keep -1 at the end.
Test version must be increased.
| 1 2 1 | ||
| 1 3 3 1 | ||
| 1 4 6 4 1 | ||
| 1 5 10 10 5 1 |
There was a problem hiding this comment.
the alignment between this row and the one above is no longer effective because 10 has two digits
|
Thanks for writing a good description of the PR ❤️ |
| 1 2 1 | ||
| 1 3 3 1 | ||
| 1 4 6 4 1 | ||
| 1 5 10 10 5 1 |
There was a problem hiding this comment.
Why do you want to add this row?
I think the first 5 rows demonstrate the triangle sufficently, and adding 2 digit numbers makes the formatting difficult. The current formatting is not correctly triangular.
There was a problem hiding this comment.
Sorry for not replying on your comment.
Ah i wrote this line for me because i wanted simply more example data and then thought it would make no big difference because somehow the alignment is nearly symmetric.
@Insti So you think it is better to leave it unchanged?
There was a problem hiding this comment.
Yes, unchanged is better.
There are MANY other easily searchable resources that provide further information on Pascal's triangle and more example rows if that is what the student wants.
|
I go back and forth on whether adding test cases that can be passed without changing any code is a good idea or not, but in this case adding the extra cases 5,6,10 doesn't seem too bad. |
|
I am kind of thinking the way I would test pascal's triangle would be to test that |
| 1 2 1 | ||
| 1 3 3 1 | ||
| 1 4 6 4 1 | ||
| 1 5 10 10 5 1 |
There was a problem hiding this comment.
when there were only single-digit numbers, it was the case that no number is directly above another.
but right now, there is a 1 directly above each 5, and there is a 4 directly above each 10
so this example is not currently showing the nature of the triangle
It is possible that this could be fixed. If it were, that would be better.
some possible solutions involve an unequal number of spaces in each gap in the same row. the resulting inconsistency raises questions about whether the relationship is different simply because of the difference in representation.
If a sixth row is to be added, such concerns should be addressed
There was a problem hiding this comment.
I agree that adding the line makes reading the triangle harder, while not providing much benefit I feel. @hekrause would you be opposed to removing the extra line?
|
@Insti I reset the description. |
There was a problem hiding this comment.
As discussed
I have given my justification for why it is appropriate to add five rows.
I have no justifications for adding six or ten rows, so this comment passes no judgment on them.
It is my belief that all rows added have the correct values.
My approval is conditional on the Squash and Merge button being used OR the commits being squashed using any other way.
|
Thanks @hekrause ❤️ |
In relation to this pull request, where it was mentioned that fewer test greater 4 rows would not be needed.
To see the structure of the algorithm I wrote myself a few additional tests because I had my difficulties with getting a grip on the "pascals-triangle" exercise.
Maybe they are not necessary to solve this exercise but they do no harm.
I implemented the tests in this pull request and now made the corresponding changes in this repository.