2
\$\begingroup\$

I have just finished an assignment and would like to know what best practices I might be missing in my code. Any help would be hugely appreciated.

import random
random.seed(42)


def alphabet_position(text_):
    def numeric_position(char_):
        # Built in method for getting the alphabet position
        import string
        return 1+string.ascii_lowercase.index(char_.lower())
    # Consider only alphabetic characters
    return ' '.join([str(numeric_position(t)) for t in text_ if t.isalpha()])


def count_smileys(smileys: list) -> int:
    import re
    pattern = r'(:|;)(-|~)?(\)|D)'
    total = 0
    # Add 1 if it matches a face (Only works in Python 3.8 or greater)
    [total := total + 1 for s in smileys if bool(re.search(pattern, s))]
    return total


class Bingo:
    def __init__(self, n):
        self.initial_size = n
        self.current_ball = None
        self.picked_balls = []
        self.remainder_balls = n

    def pick_ball(self):
        # Do not consider the already picked balls for choosing randomly
        self.current_ball = \
            random.choice([i for i in range(1, self.initial_size)
                           if i not in self.picked_balls])
        self.picked_balls.append(self.current_ball)
        self.remainder_balls -= 1


def pick_n_balls(n_):
    bg = Bingo(75)
    for i in range(n_+1):
        bg.pick_ball()
    output = f"({bg.current_ball}, {bg.picked_balls}, {bg.remainder_balls})"
    return output


def assert_everything():
    assert alphabet_position("Lorem ipsum dolor sit amet, consectetur adipiscing elit.") == '12 15 18 5 13 9 16 19 21 13 4 15 12 15 18 19 9 20 1 13 5 20 3 15 14 19 5 3 20 5 20 21 18 1 4 9 16 9 19 3 9 14 7 5 12 9 20'
    assert count_smileys([]) == 0
    assert count_smileys([':D',':~)',';~D',':)']) == 4
    assert count_smileys([':)',':(',':D',':O',':;']) == 2
    assert count_smileys([';]', ':[', ';*', ':$', ';-D']) == 1
    assert pick_n_balls(16) == (53, [15, 4, 38, 34, 31, 20, 16, 13, 63, 6, 5, 9, 22, 24, 46, 53], 59)


if __name__ == "__main__":
    print(alphabet_position("Lorem ipsum dolor sit amet, consectetur adipiscing elit."))
    print(count_smileys([]))
    print(count_smileys([':)', ';(', ';}', ':-D']))
    print(count_smileys([';D', ':-(', ':-)', ';~)']))
    print(count_smileys([';]', ':[', ';*', ':$', ';-D']))
    pick_n_balls(16)
    assert_everything()
\$\endgroup\$
1
  • 2
    \$\begingroup\$ What do alphabet_position, count_smileys and the Bingo stuff have to do with each other? It seems they're completely unrelated. Also: What are they supposed to do? Task description(s) is/are missing. \$\endgroup\$
    – no comment
    Sep 23 at 14:17
4
\$\begingroup\$
random.seed(42)

We only want to do this in the main guard, not when loaded as a module. It's really part of the test function, not the module.

total = 0
# Add 1 if it matches a face (Only works in Python 3.8 or greater)
[total := total + 1 for s in smileys if bool(re.search(pattern, s))]
return total

We can use sum() for counting:

return sum(1 for s in smileys if re.search(pattern, s))

We might want to make a separate function for is_smiley(), simply to make the tests more specific. Also consider compiling the regular expression just once, using re.compile().

On the subject of tests, seriously consider using the doctest module, to keep your tests close to the code they exercise. That would look like this for the count_smileys() function:

import re

SMILEY_REGEX = re.compile(r'(:|;)(-|~)?(\)|D)')

def count_smileys(smileys: list) -> int:
    """
    Return the number of strings in SMILEYS which contain at least one smiley face.

    >>> count_smileys([])
    0
    >>> count_smileys([':D', ':~)', ';~D', ':)'])
    4
    >>> count_smileys([':)',':(',':D',':O',':;'])
    2
    >>> count_smileys([';]', ':[', ';*', ':$', ';-D'])
    1
    """
    return sum(1 for s in smileys if SMILEY_REGEX.search(s))

if __name__ == "__main__":
    import doctest
    doctest.testmod()
\$\endgroup\$
10
  • \$\begingroup\$ Just wondering: have you ever written/seen a program exceeding the compiled patterns cache? \$\endgroup\$
    – no comment
    Sep 23 at 15:32
  • \$\begingroup\$ Actually, I'd forgotten the cache. Are you saying it's better to just re.compile() every time the function is called? I'm not an expert with re internals. \$\endgroup\$ Sep 23 at 16:27
  • \$\begingroup\$ No, I was thinking of just using re.search. But the explicit compile way does indeed look much faster, at least with such short test strings. \$\endgroup\$
    – no comment
    Sep 23 at 16:36
  • 2
    \$\begingroup\$ re.search() takes a string, which needs to be turned into a regular expression automaton (i.e. compiled) each time the function is called. re.compile() does just the compilation, and allows us to reuse the internal form rather than constructing it anew each time we want to search. I think the documentation explains that better than I could! \$\endgroup\$ Sep 24 at 14:47
  • 1
    \$\begingroup\$ @TobySpeight No, re.search and others do not compile anew each time. At the end of exactly that section you yourself linked to it says "The compiled versions of [...] and the module-level matching functions are cached, so programs that use only a few regular expressions at a time needn’t worry about compiling regular expressions". \$\endgroup\$
    – no comment
    Sep 25 at 12:55
4
\$\begingroup\$

As pointed out at stackoverflow, your method

random.choice([i for i in range(1, self.initial_size)
               if i not in self.picked_balls])

for picking the next ball is rather inefficient. Your i goes through n values and the not in check for the list takes O(n) time, so this is O(n2) for just one ball, and O(n3) for the whole game (though I'm not familiar with Bingo, don't know how many balls can be picked without anybody winning and thus the game ending early or however that works :-).

You can reduce it to O(1) per pick, for example by keeping the remaining balls in a list, picking one from a random index, and replacing it with the last ball from that list.

\$\endgroup\$
2
\$\begingroup\$
  1. Python has a built-in function that returns the code point of any given character in UNICODE, ord, which is designed to do exactly this sort of thing. We aren't manipulating strings in any way here, so there is really no need to import string here. Lower case A is 97 ('\u0061') in UNICODE, so here we only need to subtract 96 to get the index in the alphabet.

Performance testing:

In [172]: ord('z') - 96
Out[172]: 26

In [173]: ascii_lowercase.index('z') + 1
Out[173]: 26

In [174]: %timeit ord('z') - 96
63.8 ns ± 0.298 ns per loop (mean ± std. dev. of 7 runs, 10000000 loops each)

In [175]: %timeit ascii_lowercase.index('z') + 1
169 ns ± 1.13 ns per loop (mean ± std. dev. of 7 runs, 10000000 loops each)

There is nothing that can beat ord at this. Also, generally 0-based indexing (indexes start at 0) is more widely used in programming, this makes much more sense mathematically than 1-based indexing.

  1. as @Toby Speight pointed out, your code to count smileys is really poor and should be realized using sum.

However, there are also two other offenders, the incorrect usage of list comprehensions, and importing a module inside a function.

Never assign variables inside list comprehensions, list comprehension should only be used to produce lists, period.

And importing a module inside a function makes the module only known in the functions namespace, generally importing should be done at the top level of the module, so that name is globally known and all functions can use it.

Performance:

In [176]: pattern = r'(:|;)(-|~)?(\)|D)'
     ...: total = 0
     ...: smileys = [';]', ':[', ';*', ':$', ';-D']
     ...: [total := total + 1 for s in smileys if bool(re.search(pattern, s))]
     ...: total
Out[176]: 1

In [177]: sum(1 for s in smileys if re.search(pattern, s))
Out[177]: 1

In [178]: %%timeit
     ...: pattern = r'(:|;)(-|~)?(\)|D)'
     ...: total = 0
     ...: smileys = [';]', ':[', ';*', ':$', ';-D']
     ...: [total := total + 1 for s in smileys if bool(re.search(pattern, s))]
     ...: total
4.72 µs ± 32.6 ns per loop (mean ± std. dev. of 7 runs, 100000 loops each)

In [179]: %timeit sum(1 for s in smileys if re.search(pattern, s))
4.46 µs ± 46.4 ns per loop (mean ± std. dev. of 7 runs, 100000 loops each)

sum method is by far more readable.

  1. There is really no need to construct your Bingo class, and your method to choose balls is really inefficient.

What's more, you can't ensure the output to be non-random by setting random.seed(42) at the top level of the module. It is true that random.seed(42) will make the output deterministic, like this:

In [1]: import random

In [2]: random.randrange(1000)
Out[2]: 806

In [3]: random.randrange(1000)
Out[3]: 52

In [4]: random.randrange(1000)
Out[4]: 536

In [5]: random.seed(42)

In [6]: random.randrange(1000)
Out[6]: 654

In [7]: random.seed(42)

In [8]: random.randrange(1000)
Out[8]: 654

In [9]: random.seed(42)

In [10]: random.randrange(1000)
Out[10]: 654

However this will only work once, because after each output, the seed is reset so new outputs will be random, you have to reset the seed each time if you want non-random output.

In [11]: random.randrange(1000)
Out[11]: 114

In [12]: random.randrange(1000)
Out[12]: 25

In [13]: random.randrange(1000)
Out[13]: 759

Therefore the output of your original pick_n_balls(16) is completely random, with each output having exactly math.prod(1 / (75 - i) for i in range(16)) (1/75 + 1/74 + 1/73 + 1/72 ... + 1/60) = 5.5900008629879506e-30 chance.

And your original output is a str while you verify it against a tuple, the code will always throw AssertionError and it is indeed correct, str can never and should never equal a tuple.

There is also no need to return the current ball and remaining ball count, since the current ball is the last in the list and remaining ball count is simply 75 - len(ls) (because total number of balls is always 75).

random.sample always returns lists without duplicates (it will not sample elements with the same index twice), so just use it.

This is the fixed code:

import random
import re
from typing import Iterable

def alphabet_position(text: str) -> Iterable[int]:
    return [ord(t.lower()) - 96 for t in text if t.isalpha()]


def count_smileys(smileys: list) -> int:
    pattern = r'(:|;)(-|~)?(\)|D)'
    return sum(1 for s in smileys if re.search(pattern, s))


def pick_n_balls(n: int, nonrandom: bool = False) -> Iterable[int]:
    if n > 75:
        raise ValueError(f'The inputted number should be no more than 75, got: {n}')
    if nonrandom:
        random.seed(42)
    return random.sample(range(75), n)


def assert_everything() -> None:
    assert alphabet_position("Lorem ipsum dolor sit amet, consectetur adipiscing elit.") == [
        12, 15, 18, 5, 13, 9, 16, 19, 21, 13, 4, 15, 12, 15, 18, 19, 9, 20, 1, 13, 5, 20, 3,
        15, 14, 19, 5, 3, 20, 5, 20, 21, 18, 1, 4, 9, 16, 9, 19, 3, 9, 14, 7, 5, 12, 9, 20
    ]
    assert count_smileys([]) == 0
    assert count_smileys([':D',':~)',';~D',':)']) == 4
    assert count_smileys([':)',':(',':D',':O',':;']) == 2
    assert count_smileys([';]', ':[', ';*', ':$', ';-D']) == 1
    assert pick_n_balls(16, True) == [14, 3, 35, 31, 28, 17, 13, 11, 54, 4, 73, 67, 68, 74, 32, 38]


if __name__ == "__main__":
    assert_everything()

Your original code will always fail at the last assertion:

enter image description here

But I have managed to make pick_n_balls(16, True) deterministic, it will always be [14, 3, 35, 31, 28, 17, 13, 11, 54, 4, 73, 67, 68, 74, 32, 38], though it is completely pointless (unless you want to rig the games which is unethical).

enter image description here


P.S., your method to identify smileys is oversimplified, your regex can only cover 12 possible variations, I don't know a lot about ASCII art however I am not convinced that you have covered every one of them.

And there are emojis and other pictograms depicting smiles defined within UNICODE, for example this character: U+1F600, and emoticons like this: ʘ‿ʘ, and your regex approach won't identify anyone of them.

Your definition of smileys is simply too narrow, I would recommend using neural networks for such task, as they are by far more suitable for this kind of task than regexes.

There can always be areas your definitions won't cover, something you cannot forsee, using a predefined set of rules will make the program produce the correct output when the input isn't covered in the definitions of the rules.

By using ANNs, rather than process predefined rules, your program learns to predict output for any input, it adapts to the training data, so it can produce correct output for inputs you didn't forsee.

\$\endgroup\$
3
  • 1
    \$\begingroup\$ generally importing should be done at the top level of the module, so that name is globally known and all functions can use it. - The conclusion is correct but the motivation isn't, particularly. Narrow-scope symbol exposure is actually a good thing. The reason for imports at the top is a combination of convention, and easily-understood module dependencies. \$\endgroup\$
    – Reinderien
    Sep 25 at 12:17
  • 1
    \$\begingroup\$ Overall I think there are some good elements of feedback in here, but your tone needs some work. For instance, real programmers use 0-based indexing - what's to say that the OP is not a real programmer? Saying the quality of your code is really poor is maybe even true, but isn't useful to say on its own, and should be conveyed through the use of constructive examples. I think focusing on the facts and being a little bit more gentle in your tone will help your review. Further reading e.g. codereview.meta.stackexchange.com/questions/2499/… \$\endgroup\$
    – Reinderien
    Sep 25 at 12:22
  • \$\begingroup\$ Thanks a lot for taking the time to write this answer. I've taken notes and implemented some of your suggestions in my code. Thanks again! \$\endgroup\$
    – banana_99
    Sep 27 at 6:33

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

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