Ad
  • Custom User Avatar

    Approved by someone

  • Default User Avatar

    Oh, nice catch! My bad. Thanks for the fix.

  • Default User Avatar
  • Default User Avatar

    Hey, just noticed an error in the random test generator (I missed this earlier when reviewing) - we're just religiously checking values of n from 1 to 30 inclusive, instead of the actual random number generated. Extract of relevant code below. Should be testing against Kata.Pascal(randNum), NOT Kata.Pascal(i). Can this be fixed?

          for (var i = 1; i < 30; i++) {
            randNum = RandInt(1, 68);
            expected = PascalSol(i);
            Assert.AreEqual(expected, Kata.Pascal(i), "Failed a random test");
          }
    
  • Default User Avatar

    Oh! Okay. (:

    Thank you for your attention.

  • Default User Avatar

    I reckon what you've done is fine as is - it's not a performance Kata so don't think testing for large values (for either performance reasons or exceeding max value of a int64) is needed here.

  • Default User Avatar

    Oh! That is because I simply translated this kata directly from the js version without giving it much thought. That version performs 20 random tests in the range (1, 40). It probably has something to do with integer limits.

    Given the limits of native integers in C#, I took the liberty of tuning the random tests up to the larger range (1, 68) with 30 tests. Sounds better?


    Since js can only safely represent integers (natively) in the range (-2^53, 2^53), the last row it can compute without resorting to bignums is the 57th, whose largest element is ~7.6e15, while 2^53 is ~9e15. The maximum in the 58th row is ~1.5e16.

    C#'s largest native integer is a 64-bit one. The signed flavor goes up to 2^63-1 (about 9.2e18), which lets us represent elements up to the 67th row (maximum about 7.2e18), whereas the 68th row's maximum is about 1.4e19. Since the unsigned version goes up to 2^64-1 (about 1.8e19), it does not make a difference; and 67th row is the farthest one can do in C# without resorting to some extra work.

    (Hm, now that I think about it, we could also test how the program performs in the entire range instead. It finishes in 2300ms, just tried it. What do you think?)

  • Default User Avatar

    Looks good, but any particular reason (and I ask this relative to the random tests for the Python version) why you only execute 20 random tests (vs. 40 in Pyhton), and those tests only check input in the range (1, 40) (again vs. (1, 100) in Python)?

  • Default User Avatar

    C# translation available for review.

  • Default User Avatar

    Beautiful, clear, efficient solution.
    Love the way you avoided writing small loops using Linq and string manipulation: you achieve code readability without losing efficiency.
    The only for loop remaining was unavoidable, and yet the code remains very clean. Congrats.