4
\$\begingroup\$

I was working on Python Crash Course, exercise 9-15: 'Lottery Analysis'. It took me a very long time to get it to work. The reason it took that long is I wanted to use my existing class from the previous exercise 9-14 which used choices() method not choice(). I have looked at the author's and some other solutions but still don't get the logic behind those solutions, especially incrementing and then showing the number of attempts it took to win. I have looked at the book's material over and over again and still couldn't get it to work using choice() on my own. Until I found a few articles talking about different methods for using random. I went over those articles and finally got the other exercise 9-14 to work.

First, I want to know what I did is normal for anyone who just started learning Python with no coding background. Am I complicating things to work this way? How can I get my code, see below, to work using choice()? Can the following code be simplified with fewer lines? Any advice is much appreciated!


Full code

from random import choices


class WinningTicket:
    """
    Randomly select characters to generate tkt numbers.
    Take a tkt no. and see how many tries it will take to win.
    """

    def __int__(self, selection, final):
        """Initializing attributes."""
        self.selection = selection
        self.final = final

    def generate_tkt(self, length):
        """Randomly select a series of characters to generate tkt num."""
        series = ["a", "b", "c", "d", "e", 0, 1, 2, 3, 4, 5, 6, 7, 8, 9]
        self.selection = choices(series, k=length)
        self.final = "".join(map(str, self.selection))
        return self.final

    def pull_tkt(self, my_tkt):
        attempt_num = 0

        while self.generate_tkt(len(my_tkt)):
            attempt_num += 1

            if self.final == my_tkt:
                print(f"\nYou've won with tkt no. {self.final}")
                print(f"It took {attempt_num} tries to win.\n")
                break
            else:
                print(f"Trying to win: attempt no. {attempt_num}")

Create obj and call method

tkt_num = WinningTicket()
tkt_num.pull_tkt("45da")

Results

.
.
.
.
Trying to win: attempt no. 4138
Trying to win: attempt no. 4139
Trying to win: attempt no. 4140
Trying to win: attempt no. 4141
Trying to win: attempt no. 4142
Trying to win: attempt no. 4143

You've won with tkt no. 45da
It took 4144 tries to win.
\$\endgroup\$
1
  • 4
    \$\begingroup\$ If you want to include the suggestions from the answers into your code and get another review ask a follow up question with a link back to this question. Please do not edit the question, especially the code, after an answer has been posted. Changing the question may cause answer invalidation. Everyone needs to be able to see what the reviewer was referring to. What to do after the question has been answered. \$\endgroup\$
    – pacmaninbw
    Commented Jul 6, 2023 at 13:34

4 Answers 4

6
\$\begingroup\$

Without knowing exactly the problem you're trying to solve, I will probably miss some points, but I'll try a review anyway.

Style

Your code is very clean and pretty well documented, it's a pleasure to read. Kudos for that.

Typos/unintended behavior/bug

You define an __int__ magic method. Judging by the docstring and code, it is probably intended to be __init__. However, __int__ is a valid magic method, intended for casting the class to an integer, introducing a lot of confusion.

Moreover, since your code doesn't define an __init__ initialization method, a default one is used, which doesn't take any parameters (besides self). This is used in your example usage: tkt_num = WinningTicket(). Should you fix the typo in "init", this line would raise an exception, as the initializer now requires 2 more parameters.

Naming

Your naming could be improved. WinningTicket is an odd name for the class, as it does not really represent a ticket, and definitely not always a winning one. Lottery would be better IMO.

You shouldn't use abbreviations in names, unless they are very common ones. Spell out "ticket"; the 3 characters saved by writing "tkt" are definitely not worth the hit on readability.

Constants

You use hard-coded constants for the item pool used in the lottery. This should be a named constant for readability and modifiability. It should probably be defined outside of the method, and either passed as a parameter or set and accessed as an class member.

Finally, since string are iterable in Python, you can use a single string instead of an array of characters and integers, which would simplify the code down the line, saving a call to map

print in a loop

Your pull_tkt method repeatedly prints inside a loop that runs many times, and that should run fast.

There are a couple of issues with that:

  • This violates separation of concerns.
  • It spams the output with output containing no information. If you are only given the final line of the output, "It took 4144 tries to win.", you can infer it missed 4143 times. There is absolutely no gain in writing that you tried that many times. However, it buries the relevant information in noise. Worse, assuming you output on a terminal, if you run the simulation more than once, the important information will be deleted to make room for the noise by the time the next simulation terminates, and will be lost forever.
  • print is slow (as a general rule of thumb, IO is slow, whatever kind of IO on whatever language). Since the loop runs many times, you would want it to run as fast as possible, it could save minutes or hours on longer tickets.

Incorrect use of classes

There is no need for a class as you use it. As it stands, all functionality could be provided by two functions:

from random import choices


def generate_ticket(length, pool):
    """Randomly select a series of characters to generate ticket number."""
    return "".join(map(str, choices(pool, k=length)))


def simulate(pool, target):
    attempts = 0

    while generate_ticket(len(target), pool) != target:
        attempts += 1
    return attempts


POOL = '123abc'
my_ticket = generate_ticket(3, POOL)
print(f'It took {simulate(POOL, my_ticket)} tries to win.')

No need to instantiate classes or keep state in members.

\$\endgroup\$
1
  • \$\begingroup\$ Thanks for your time in provided a very helpful and constructive feedback \$\endgroup\$
    – snapieee
    Commented Jul 14, 2023 at 2:15
5
\$\begingroup\$

It's a simple project, so a short review. This is one I honestly wouldn't bother using a class for, I think that this would be better implemented as a pair of functions, but it's good practice and doesn't hurt.

Typo

First off, major issue. The constructor dunder (double underscore method) is __init__, not __int__ as you've got here, the __int__ method provides a means of converting the value to integer.

This (presumably typo) is actually the only reason your code as written is not crashing, as you have mandatory arguments (selection and final) which you aren't providing. Also, you would never want to set these values as they're immediately overwritten in generate_tkt

Constants

Your constant series is something which you may want to use in your __init__ method. This would allow you to define the "rules" for your ticket draw. The other thing is that since it's a constant, you can make the types whatever you wish. Don't use integers and then change them to strings. Use strings directly. In fact, random.choice can pull from a string directly, so we just need "abcde0123456789"

class WinningTicket:
    """
    Randomly select characters to generate tkt numbers.
    Take a tkt no. and see how many tries it will take to win.
    """

    def __init__(self, series="abcde0123456789"):
        """Initializing attributes."""

        # May wish to add extra checks on validity of series
        # or simply rely on duck typing
        self.series = series
        self.selection = None
        self.final = None

Either that or, if it's consistent throughout, you may want to make it a class-level constant (i.e. put it inside the class block) so that all instances share a copy, or as a module-level constant.

Naming

The name WinningTicket doesn't actually describe what the class does, it is not a winning ticket itself, it may result in one, but it is not one. A better name might be Lottery as this is the process of generating and drawing the tickets.

You may also want better attribute names. final is not very descriptive of what the variable is/does and though in this instance it may be obvious, it may be better to call it last_drawn or similar.

Possible Extensions

  • Checking whether the user entered ticket number will actually occur if I ran:
tkt.pull_tkt("zzzz")

Your code would run forever.

  • Being able to create multiple tickets and see which wins.
\$\endgroup\$
1
  • \$\begingroup\$ Thanks for your time in provided a very helpful and constructive feedback \$\endgroup\$
    – snapieee
    Commented Jul 14, 2023 at 2:15
1
\$\begingroup\$

Random

Let's talk about randomness. Your lottery draw does nothing to prevent repeat ticket draws.

This is a very rough and incomplete summary of your method compared to some other methods available.

Method Security Repeats Memory In-built
random.choices Poor Yes Excellent Yes
secrets.choice Good Yes Excellent Not quite (needs wrapping)
secrets.choice with iterative set-building Good No Poor Yes
LFSR Very poor No Excellent No
random.shuffle of entire sequence Poor No Terrible Yes
secrets shuffle of entire sequence Good No Terrible No

For something claiming to be Lottery Analysis, I would expect some research into this topic; I encourage you to do some reading.

\$\endgroup\$
1
  • 1
    \$\begingroup\$ Thanks for your time in provided a very helpful and constructive feedback \$\endgroup\$
    – snapieee
    Commented Jul 14, 2023 at 2:15
1
\$\begingroup\$

Simplify the problem

Unless the assigned problem is specifically to simulate the lottery in precise detail, you should start by simplifying the problem.

The first step, rather than generating four characters from a set of fifteen with replacement, you are better off generating a single number from a space of 50625 (\$15^4\$). This number can easily be converted to the string of four characters, but is a lot easier to deal with.

Second, an analysis won't care what number you start with as your target. Pick 0 or 1 (depending on if your random number starts at 0 or 1).

and go further

Third, to be doing an analysis you really want to do more than one target trial. One could easily try all 50625 possible targets at once, either to one match each or to see what happens in a few million rolls. I'm not sure if either of these will produce the same results as sequential trials, but it would make in interesting test.

At this point, you can start doing statistical analysis on the results. What is the mean number of rolls to get a win? What's the standard deviation?

If you can get to a point where multiple runs produce comparable results, congratulations. (If you get the same result, you've done something wrong, like fixed-seed your random number generator.) It can be worth trying with a secure random number generator (do this last, it is slower), and might provide a test of the quality of a pseudo-random number generator.

\$\endgroup\$

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge you have read our privacy policy.

Not the answer you're looking for? Browse other questions tagged or ask your own question.