Draft

Find Parallel Perfect Fifths

9Simple Rick
Description
Loading description...
Algorithms
Fundamentals
Performance
View
AllIssues2Questions1Suggestions2Show Resolved
  • Please sign in or sign up to leave a comment.
  • hobovsky Avatar
    • Tests call sol twice per test case, which is unnecessary.
    • Tests use the variable harmony after it was potentially mutated by user solution, what can lead to confusing messages or tests working incorrectly.
    • Tests should not print to stdout if not necessary. Information usually looks better when presented with test headers, with custom assertion messages, if the message is expected to be short.
    • Suggestion: return values of "Passed"/"Failed" scream for boolean and not strings.
    • Question: your tests seem to generate test cases as failing or passing upfront. Is it even necessary to call the reference solution?
    • Simple Rick Avatar

      Thank you!

      Bullet 1. removed console logs that were calling sol a second time. I think that's what you meant?

      Bullet 2. I don't understand this really. It's been my experience that if I don't use actual and expected in this way that people can mutate the array to cheese the test. Any suggestions for how to fix what you're describing?

      Bullet 3. removed all console logs.

      Bullet 4. Point taken. I'll consider changing it.

      Bullet 5. Not exactly. The variable named passing is there to help even out the ratio of passing and failing harmonies generated. However it being false does not necessarily mean a completely random harmony won't pass, it's just rare that it does.

    • dfhwze Avatar

      Bullet 2: it's good that you calculate the reference solution before user input, however, you are still using the same variable 'harmony' you provide to the user in the assert message (possibly altered by that user). You can solve this by providing the user with a copy of that variable instead.

  • dfhwze Avatar

    Looking at similar kata's, I would add the following tags: algorithms, fundamentals

  • dfhwze Avatar

    I'm timing out on the first bigger random test. Are you sure there aren't any performance constraints? If so, add the "performance" tag.

    • dfhwze Avatar

      Ok, we needed to check consecutive chords, not any combination of chords. But still, I get close to a timeout after fixing this.

    • Simple Rick Avatar

      The reference solution also takes about 10 seconds to pass. Given that the users solution must be about as fast or faster than the reference solution in order to pass the long random tests. I've added the 'Performance' tag to the Kata.

      Suggestion marked resolved by Simple Rick 3 years ago
    • dfhwze Avatar

      In addition to the tag, I would also add a note at the end of kata's description stating something about the performance criteria. For instance, the number of tests, or the number of chords in big tests.

    • Simple Rick Avatar

      Okay. Added: (99 of your students will turn in harmonies around 1000 to 50,000 chords long) to the description

  • scarecrw Avatar

    Am I missing something in this random test?

    Testing for  re ti re fa,re la fa sol,ti mi do mi,do re re mi,fa mi re sol: expected 'Fail' to equal 'Pass'

    It seems to me the soprano and the bass have a perfect fifth on the 2nd and 3rd chord:

    re la fa sol    sol -> re
ti mi do mi     mi  -> ti
    • Simple Rick Avatar

      The bass and soprano are a perfect Fourth apart. The Soprano sings higher than the Bass, so the interval is a fourth, not a fiftth. I hope I'm explaining that clearly.

      When considering an interval you consider the lower pitch note against the higher pitched note, not the reverse.

      I'll try to think of a way to concisely explain that in the description.

    • scarecrw Avatar

      Thanks for clarifying; description looks good!

      Question marked resolved by scarecrw 3 years ago
  • Voile Avatar

    The remark about diminished fifths is unclear. What exactly is a diminished fifth? Only ti to fa?

    Also, this should be put in the fixed tests as well.

    • Simple Rick Avatar

      Yes. In the major scale, the only diminished fifth is ti to fa. All of the other fifths are perfect fifths.

      I added:

      ti to fa is the only case in which the interval is a fifth and not a Perfect Fifth.

      to the description

      Hope that helps?

    • Simple Rick Avatar

      Okay. I'll make a fixed test.

      A perfect fifth is when they are 8 semitones apart. A diminished fifth is only 7 semitones. That's why it's not perfect.

      Not sure how deeply to delve into all that business in the description, or if what I added will suffice.

    • Simple Rick Avatar

      Added a fixed test for the special case of 'ti' and updated description

      Issue marked resolved by Simple Rick 3 years ago