Ad
  • Custom User Avatar

    See a possible bootstrap, up there ?

  • Custom User Avatar

    Alright, looking good.
    One note for future translations: when dividing tests into "big" and "small" inputs (which have an impact on expected runtime), try to give the "small" tests a lexicographically "early" name (i.e. starting with 'a', 'b'... etc.), and the large tests a relative "late" name (starting with 'm'..'z' etc). While the tests are officially run in parallel, in reality the test harness runs them alphabetically. In this case, you want small and easy tests to complete before a larger tests potentially times out, so the user gets better feedback.
    Not important here, so I'm approving as is.

    Cheers.

  • Custom User Avatar
  • Custom User Avatar

    Not to belabour the point, but this is exactly what I mean by it being unnecessary:

        fn test_negative() {
            assert!(factorial(-1).is_none(), "For number -1: expected None");
            assert!(factorial(-10).is_none(), "For number -10: expected None");
        }
    

    As in, remove those 2 (!) fixed tests, and the whole "negative inputs, Option output" thing becomes completely irrelevant. Honestly, just get rid of them, make the output a string and the input a u32.

    While this may go against whatever the description says, or diverge from other language translations, there is simply no justifiable reason for it being there in the first place, and it changes absolutely nothing about the task if it's left out. Let's keep things simple, eh?

  • Custom User Avatar

    I have fixed the issues, you've mentioned. Judging by the Kata description I figured, I should stick to Option. I added a small test for this.

  • Custom User Avatar

    Personally, I don't see any point in supplying negative integers (aka invalid input), the task is to figure out how to do manual factorization with big numbers, not "distinguish whether a number is positive or negative". It adds nothing useful to the task.

  • Custom User Avatar

    Thanks for your remarks.

    Regarding the C++ solution...

    First of all, I don't know who wrote it because it is written as a part of Test Cases section. I rewrote it closely to the original for the test cases, and generalized it myself for the solution. If you think that I should credit the author of the C++ translation, I can do it.

    Apparently, it doesn't check for negative values and it uses fixed arrays, knowing, that the tested values will be under 500. I assumed, if it is already aproved, then it must be OK to do it like that as well. Otherwise, these issue also applies to the C++ translation.

  • Custom User Avatar

    Notes:

    • The Solution Setup has a syntax error (: instead of ->)
    • Please add proper assertion messages to each assertion, clarifying that left refers to the user's solution and right to the expected solution.
    • If the reference solution is copied from another user (I can't verify, I'm going off the name you gave it), then please simply credit the user with a comment, but otherwise give the function a more sensible name.
    • You are not testing negative numbers, which is why your reference solution is able to return a String instead of an Option<String>. It obviates the need for Option, but the description mentions the existence of such inputs. Either the Option needs to go, or negative numbers added to tests.
  • Custom User Avatar

    This comment is hidden because it contains spoiler information about the solution

  • Custom User Avatar

    because the C++ translator was not qualified enough

  • Custom User Avatar

    Yeah, but why does the task itself requires you to return char*?

  • Custom User Avatar

    because not everybody is qualified in C++

  • Custom User Avatar

    C:

    • unhelpful assertion messages
    • memory model is not explained
    • const qualifiers are missing
  • Custom User Avatar

    I don't understand why C++ solution uses raw pointers and strcmp. Why not std::string?

  • Custom User Avatar

    Good catch. Fixed in fork.

  • Loading more items...