Ad
  • Custom User Avatar

    Needs random tests and edge case tests. Might also be worth restructuring the tests, the current output is a bit cryptic. The Python tests provide a nice output/structure if you want to copy that.

  • Custom User Avatar

    I'm a mender so I can self approve, now that someone has looked at it I'm confident to.

    I just don't like approving my own new translations without first having someone else check them.

  • Custom User Avatar

    Ah, I'm too low in Honor to be able to approve your translation. :-(

  • Custom User Avatar
  • Custom User Avatar
  • Custom User Avatar

    Thank you very, very much for taking the time and explaining! You were right, the tests were wonky and did not check for performance. I tried to fix those issues now. The test failure messages will read like

    Expected 5 but got 3 for the following array:

    Array(1, 2, -3, ...)

    I'll put much more care into my next translations. :-)

  • Custom User Avatar

    Should have the inputs in the assertion messages for the users convenience so they don't need to print them. I would add it on a newline after your current assertion message, something like "\nInput: arr = $arr" and just hardcode it for the fixed tests.

    Your random test generation is also wonky: There's still only one random test, and Random.nextInt(9999) generates numbers from 0-9998 inclusive, thereby making the maximal subarray sum always the entire array. You can use Random.between(-9999, 10000), for example, to generate numbers from -9999 to 9999 inclusive. I would also personally use random array lengths. I usually make a helper function to generate individual test cases and solutions for me, then do:

    LazyList.continually(makeTestCase).take(100).foreach {
      case (input, expected) => assertResult(expected, <clue here>) {solution(input)}
    }
    

    to run multiple tests.

    I haven't tested yet, but have you checked that O(n^2) solutions pass your random test cases as they do in other languages? At the very least using Kadanes alg (O(n)) isn't enforced for Python, which is I believe the originally authored language for this kata.

  • Custom User Avatar

    Glad to have someone review my code! So, now I added newlines, grouped the basic tests and added a random test again. WDYT?

  • Custom User Avatar

    The changes so far look okay to me, now you just need a random test block now and it should be good, though with assertResult you can wrap all of your tests into one block like so:

    "sequence" should "pass basic tests" in {
      <all your assertResult calls go here>
    }
    
    it should "pass random tests" in {
      import scala.util.Random
      <generate and test random inputs here>
    }
    

    Add a newline to the start of the assertion messages as well, it isn't done automatically by scalatest for whatever reason, and if using the above pattern it is best practice to also add the input as well.

    Glad to have someone else working on scala translations as well.

  • Custom User Avatar

    Thanks for the review and your suggestions! I tried to address most of them.

  • Custom User Avatar

    You have several issues here, the three main ones being that:

    • you have one random test
    • the random test is in fact not random as you are seeding the random generator instance with the value 100 every time, never give a static seed to the generator unless explicitly required by the kata specs
    • Scala 3 translations/kata should always use top level method definitions unless required by kata specs (eg kata is asking for an object/class), it's entirely pointless to put them into an object

    Other than that these are more style suggestions:

    • assertResult(<expected value>, <clue string>) {<user test result>} is preferred over using matchers due to nicer assertion messages
    • spamming test groups for single tests is not needed and runs pretty slowly, its better to roll your tests into groups, this makes it easier for the user to find the exact test at which they failed when combined with the above assertion style
    • Array.fill exists, you don't need to use a for comprehension to generate your tests
    • reference solutions are greatly preferred to not be used, even if private, and especially in the main test class scope
    • you (usually) don't need to do any of your imports outside of the test class aside from importing the suite itself

    You can look at some of my translations for examples if the above points are unclear, or just ask here and I'll try to explain better

  • Custom User Avatar

    Completing this Kata taught me some more Scala goodness. Yay!

  • Custom User Avatar

    Didn't know that pattern matching could help here as well. This is a great and concise solution!

  • Custom User Avatar

    Awesome, I really like how this avoids unnecessary passes over the array!