6 kyu

52 Card Pick Up

108rdtheriault
Description
Loading description...
Puzzles
View
AllIssues4QuestionsSuggestions1Show Resolved
  • Please sign in or sign up to leave a comment.
  • KayleighWasTaken Avatar

    Typo in description - ""GB" -> 'GB'

  • natan Avatar

    Haskell translation, same as python. If something changes about the python one, lmk so I can update. I did crank up test count to 200. Don't tell anyone. Shh.

  • dfhwze Avatar
    • rdtheriault Avatar

      This comment has been hidden.

    • natan Avatar

      It has some invalid suits E R T and ranks A B C, I don't see how you could test that in a meaningful way though. Sure, could include garbage with those possible values, but, you can always pick some other invalid ranks/suits

    • rdtheriault Avatar

      I see what you mean. Given the rules of garbage only being GB, Na, and then fake cards with an "a" added at the end, it is going look for cards that can't exist. Is there something you want me to change? It does follow the rules abliet with looking for cards that don't exist. Thank you for the input.

    • dfhwze Avatar

      I think since you mention only "GB" is garbage, this solution should pass after all.

      Issue marked resolved by dfhwze 13 months ago
  • natan Avatar

    In my head, "multidimensional array" communicates that all rows are the same width. They're not.

    Also, later "nested lists" is used. Would be nice to stick to the same kind of terminology throughout. I also don't know what information that line wants to convey, "All nested lists contain strings." .. since it doesn't tell me anything it seems redundant, but maybe it means to say something specific but I have no clue what because I don't know which particular words are important. Is it "all"? "contains" does that promise there is at least one? or that there may be strings, does zero strings count as being contained? does it mean there can be other things too? didn't you already state earlier that it's a multidimensional array of strings? I'm... confused.

    oh and also: all your uses of random.shuffle is redundant. you either shuffle it again later or the data already has no order.

    there should probably be at least one test that has all good cards once or more but with no junk. for a particular kind of solution it's very easy to introduce an off-by-one bug that requires there to be junk in the input.

    • rdtheriault Avatar

      I had feedback when I posted this before that I should have all the elements in the nested lists be strings. I was trying to point that out. Will rewording it to "All elements in the nested lists will be strings" make more sense.

      Would just using "a list of random length lists" be better comminucation vs "multidimensional array"?

      Thank you for your feedback.

    • Voile Avatar

      Usually they're called "jagged array".

    • natan Avatar

      or rows of varying length, dropping the jargon entirely

      I still don't understand what that's supposed to say. If you mean they can't be empty then.. why not, they're of varied length anyway. or if you mean other types, well, you shouldn't be mixing anyway so there's nothing to say there. it's also not a rule that I need to follow .. maybe it says something, but I suspect it doesn't need saying, and maybe I'm stupid but if I can't figure out what it's supposed to say then it's probably not going to do a good job for others either. even worse, they might interpret it as saying something you didn't mean to say, where I only interpret it as saying nothing.

    • rdtheriault Avatar

      Ok, I deleted the reference to array of strings as it should be implied like you mentioned. I updated the other line to I have "thrown" your deck of cards into a "pile" (a list with rows of varying length).

      Thanks again for the help.

    • natan Avatar

      Your add_a_randomly looks.. sus. Maybe instead directly state the chance for each outcome? Something like:

      def chance_out_of_1(options, default):
    weights = [w for w, _ in options]
    if sum(weights) > 1:
        raise ValueError("Can't exceed 100% chance")
    default_weight = 1.0 - sum(weights)
    weights.append(default_weight)
    values = [v for _, v in options] + [default]
    return random.choices(values, weights)[0]

# I didn't take much care in working out the probabilities that were in your code,
# so these might be off - but I don't think what you wrote is what you meant anyway :X

def maybe_sabotage(key):
    return chance_out_of_1([
        (0.0045, ["GB"]),
        (0.00385, [key, "GB"]),
        (0.04455, [key, key + 'a']),
        (0.04455, [key, key])
    ], default=[key])
    • natan Avatar

      Is pile5 meant to be what I suggested? I meant that the content would be only cards. Because if replacing >= with > in mauro-1's solution or in my solution le with lt, then it's wrong but passes and it's an easy mistake.

          # fresh out of the packaging, minus the jokers
    pile6 = [
        ['S1', 'S2', 'S3', 'S4', 'S5', 'S6', 'S7', 'S8', 'S9', 'S10', 'SJ', 'SQ', 'SK'],
        ['D1', 'D2', 'D3', 'D4', 'D5', 'D6', 'D7', 'D8', 'D9', 'D10', 'DJ', 'DQ', 'DK'],
        ['C1', 'C2', 'C3', 'C4', 'C5', 'C6', 'C7', 'C8', 'C9', 'C10', 'CJ', 'CQ', 'CK'],
        ['H1', 'H2', 'H3', 'H4', 'H5', 'H6', 'H7', 'H8', 'H9', 'H10', 'HJ', 'HQ', 'HK']
    ]
    • natan Avatar

      you should not import * because the solver may define things with names that you then end up using but thinking it's something else. only import the solution itself so that you're in control of your namespace :x

    • rdtheriault Avatar

      I'm sorry, I don't see where import * is.

      I added your fresh package, thank you for that.

      I also updated the random code to do and explain the percentanges better.

      Am I on the right track?

    • natan Avatar

      Ah, sorry, I was describing that in a weird way -

      codewars adds from solution import * if you don't import the solution yourself, which should be from solution import pick_em_up

      I DON'T like how you wrote those probabilities because.. the chances there are:

      0.01 * 0.99**0
0.01 * 0.99**1
0.01 * 0.99**2
0.01 * 0.99**3

      on the other hand it's simple so I can't really argue with it. the numbers are arbitrary anyway. the nested if-statements is what really bothered me. I don't need to like it. that's fine.

    • rdtheriault Avatar

      Ok, I added "from solution import pick_em_up". I did not realize that is how it worked. Yes, I didn't like the nested if either. I did know the percents were not perfect and thought about doing the math for it but as you said, they are rather arbitrary and I needed some random statements that worked.

      Thanks again for all your help.

  • hobovsky Avatar

    Fixed tests do not show inputs, only random tests do. Ideally, every fixed test you do would have its own @it, because after all they test for something else.

    • rdtheriault Avatar

      Thank you for the feedback, I updated the test cases, should I do the example tests as well?

    • hobovsky Avatar

      For example tests, it depends on what kind of UX you want to provide to users. There is no strict recommendation for example tests because users can see them anyway, and can easily infer what happens and why. Users can't see what happens in the ATTEMPT tests, so the feedback needs to be more explicit.

  • Voile Avatar

    Python test is incorrect set up, assertions in random tests are properly wrapped inside describe and it but the fixed tests do not.

    • rdtheriault Avatar

      Thank you for your feedback. I have moved the Python test into a describe. I also moved the other functions into the random tests.

      Thank you for the clarification, I must have misunderstood the documetation.

    • Voile Avatar

      Test assertions in sample tests is only wrapped with a describe block, the it block is missing.

    • rdtheriault Avatar

      Thanks again, sorry for missing that. It should be fixed now.

    • Voile Avatar

      This is not how it should be applied:

          @test.it('Regular cases')
    test.assert_equals(pick_em_up(pile1),True)
    test.assert_equals(pick_em_up(pile2),False)

      Just follow the ones from the actual tests.

    • rdtheriault Avatar

      I apologize again, that took me a lot longer than it should have to see the missing def. It now matches the random tests.

    • Voile Avatar
      Issue marked resolved by Voile 13 months ago
  • HerrWert Avatar

    For years, people have told me I'm not playing with a full deck. Now we finally have a way to determine whether that is true! Thanks for the kata.

  • hobovsky Avatar
    • Tests are susceptible to mutation of input: assertion messages present potentially destroyed input lists on failure.
    • There's not much sense to call a group of tests "Small random tests" if there is no Large random tests :)
    • rdtheriault Avatar

      Thank you for the feedback, I changed the name to 100 random tests to accurately indicate what is happening.

      If I understand the mutation issue, I added deepcopy to each deck (list) creation to address the issue. (let me know if that is what you where mentioning)

      Thank you again for your feedback

    • hobovsky Avatar

      I think you make the copy in not a right way. This solution destroys input and makes assertion messages wrong:

      def pick_em_up(pile):
    pile.clear()
    return False

      Additionally, fixed tests should show inputs too, not just random tests.

    • rdtheriault Avatar

      Thank you again, I think I see it now. I made the copy in the wrong spot. I needed to copy it in the test not the creation. Let me know if this works (I tested the clear and it no longer shows as blank).

    • hobovsky Avatar

      It seems to work now.

      Issue marked resolved by hobovsky 13 months ago