Hamming: Add a test case to avoid wrong recursion solution

[tqa236]

Add an extra test case about one empty strand based on the discussion in exercism/haskell#796 to avoid wrong recursion solution.

[cmccandless]

What's interesting about this test case is that is far more likely to fail in Haskell (and other languages that support pattern matching on function parameters) than other languages (that don't). I was trying to think of a solution similar to the example provided in exercism/haskell#796 without using pattern matching... it seems a bit contrived, but possible?

def distance(strandA, strandB):
    if len(strandA) == 0 and len(strandB) == 0:
        return 0
    headA = strandA.pop(0)  # This would raise an unexpected IndexError in the new test case
    headB = strandB.pop(0)
    if len(strandA) != len(strandB):
        raise ValueError()
    return (1 if headA != headB else 0) + distance(strandA, strandB)

Still a good test case though!

I wonder: is test case disallow second strand longer now redundant?

[rpottsoh]

I wonder: is test case disallow second strand longer now redundant?

Or disallow first strand longer for that matter. Help me understand how these two cases are not enough?

regarding the example code in @cmccandless comment:

Why is the length equality of the strands not checked first before anything else? I am not a FP person so I would like to understand better what is happening in Haskell that makes what looks like a redundant change necessary. Is this an issue that really affects all tracks?

[tqa236]

In my example Haskell solution in exercism/haskell#796, there is a subtle implicit assumption that both strands are not empty, otherwise, they cannot be divided into (first element : the rest) to do recursion.
This happens before the length check because that's Haskell syntax.

distance (x:xs) (y:ys)

As @sshine mentioned in the issue above, this case affects functional languages more than others. But I don't know enough about the syntax of other functional languages to comment.

And I agree that if this test case is accepted, disallow second strand longer is redundant.

[rpottsoh]

I think the other two cases should remain, even though see seem redundant. They are not redundant to each other. Perhaps the new case should be stated this way:

"description": "disallow left empty strand",
"property": "distance",
  "input": {
    "strand1": "",
    "strand2": "G"
  },
"expected": {"error": "left and right strands must be of equal length"}

then also add:

"description": "disallow right empty strand",
"property": "distance",
  "input": {
    "strand1": "G",
    "strand2": ""
  },
"expected": {"error": "left and right strands must be of equal length"}

We could also, optionally, change the error message to be more informative. ie. left strand must not be empty and right strand must not be empty

[tqa236]

Thank you for your review and suggestion. Should I push a new commit to this pull request or wait for some kind of final decisions?

[rpottsoh]

There is no harm in waiting. It would be interesting to get @petertseng's and @ErikSchierboom's opinions.

[sshine]

Sure,

1. Fix whitespace
2. Add the second test case
3. Name them as @rpottsoh suggested

(A sneeze made me click the wrong button, sorry.)

[ErikSchierboom]

Although I think this particular problem is not that likely to occur in other languages, I don't much mind the addition of the two new test cases. I would go with @rpottsoh' suggestions: adding two new test cases and giving them an error message stating that the input must not be empty.

[sshine]

It is just as likely to occur in Standard ML and Ocaml. So I encouraged @tqa236 to submit this here first.

I would also encourage a lower entry bar for new contributors. :-)

[ErikSchierboom]

I would also encourage a lower entry bar for new contributors. :-)

I'm not entirely sure I understand what you mean by this comment :)

Just to be sure, I think the appropriate action here is the list you mentioned earlier:

• Fix whitespace
• Add the second test case
• Name them as @rpottsoh suggested

Would that work for you @tqa236?

[tqa236]

It’s ok for me. I will do those 3 tasks you mentioned. Please wait for a few days

[tqa236]

I updated as you said. Please review it.

[rpottsoh]

I think the error message ought to convey why there is an error with as much useful information as possible.

[rpottsoh]

@ErikSchierboom still OK with PR?

[ErikSchierboom]

@rpottsoh 👍

[ErikSchierboom]

Thanks @tqa236!

[sshine]

I would also encourage a lower entry bar for new contributors. :-)

I'm not entirely sure I understand what you mean by this comment :)

Sorry I never got back to this, @ErikSchierboom.

I thought that the discussion of keeping/merging redundant cases might evolve so that it encompassed the original issue, discouraging @tqa236 from finalizing the change on the Haskell track. Similarly, in the hope of a returning contributor, I brought Hamming on the Haskell track up to 2.2.0 in exercism/haskell#797 while waiting for this PR.

I'm sorry if this left an impression of bullying or arguing "too many cooks" when we're generally happy to see people participate in the problem-specifications issues. (I haven't been able to keep up with this participation myself.)

[ErikSchierboom]

Right, thanks for the explanation! I was just unsure how to interpret said statement, no harm done :)