Raindrops in Python

Question

There's this simple coding challenge that feels like a twist on FizzBuzz. Your task is to convert a number to a string, the contents of which depends on the number's factors.

• If the number has 3 as a factor, output 'Pling'.
• If the number has 5 as a factor, output 'Plang'.
• If the number has 7 as a factor, output 'Plong'.
• If the number does not have 3, 5, or 7 as a factor, just pass the number's digits straight through.

For example, the sample output would look like this:

28's factors are 1, 2, 4, 7, 14, 28. In raindrop-speak, this would be a simple "Plong".

30's factors are 1, 2, 3, 5, 6, 10, 15, 30. In raindrop-speak, this would be a "PlingPlang".

I've come up with a working solution (based on the test suite results). However, I'm not really happy with it, as I feel this could be simplified. Thus, I'd appreciate some feedback.

My solution:

SOUNDS = {3: "Pling", 5: "Plang", 7: "Plong"}
FACTORS = (3, 5, 7)

def is_divisible_by(number, divisior):
    return number % divisior == 0


def raidndrops(number):
    return [SOUNDS[factor] for factor in FACTORS if is_divisible_by(number, factor)]


def convert(number):
    speak = raidndrops(number)
    return "".join(speak) if speak else str(number)

The test suite is here:

import unittest

from raindrops import convert

class RaindropsTest(unittest.TestCase):
    def test_the_sound_for_1_is_1(self):
        self.assertEqual(convert(1), "1")

    def test_the_sound_for_3_is_pling(self):
        self.assertEqual(convert(3), "Pling")

    def test_the_sound_for_5_is_plang(self):
        self.assertEqual(convert(5), "Plang")

    def test_the_sound_for_7_is_plong(self):
        self.assertEqual(convert(7), "Plong")

    def test_the_sound_for_6_is_pling(self):
        self.assertEqual(convert(6), "Pling")

    def test_2_to_the_power_3_does_not_make_sound(self):
        self.assertEqual(convert(8), "8")

    def test_the_sound_for_9_is_pling(self):
        self.assertEqual(convert(9), "Pling")

    def test_the_sound_for_10_is_plang(self):
        self.assertEqual(convert(10), "Plang")

    def test_the_sound_for_14_is_plong(self):
        self.assertEqual(convert(14), "Plong")

    def test_the_sound_for_15_is_plingplang(self):
        self.assertEqual(convert(15), "PlingPlang")

    def test_the_sound_for_21_is_plingplong(self):
        self.assertEqual(convert(21), "PlingPlong")

    def test_the_sound_for_25_is_plang(self):
        self.assertEqual(convert(25), "Plang")

    def test_the_sound_for_27_is_pling(self):
        self.assertEqual(convert(27), "Pling")

    def test_the_sound_for_35_is_plangplong(self):
        self.assertEqual(convert(35), "PlangPlong")

    def test_the_sound_for_49_is_plong(self):
        self.assertEqual(convert(49), "Plong")

    def test_the_sound_for_52_is_52(self):
        self.assertEqual(convert(52), "52")

    def test_the_sound_for_105_is_plingplangplong(self):
        self.assertEqual(convert(105), "PlingPlangPlong")

    def test_the_sound_for_12121_is_12121(self):
        self.assertEqual(convert(12121), "12121")


if __name__ == '__main__':
    unittest.main()

Answer 1

Modular division is a well-known concept to almost everyone who has done any programming. In my opinion delegating it to a is_divisible_by function is not needed and only introduces unnecessary overhead by generating an additional function call. It's not like you are ever going to use any other implementation than using modular division. Instead I would simply inline it.

While I am a fan of clear variable names, and PEP8 recommends against single letter variables, using n for a generic number (and i for a generic counting integer) is IMO acceptable and helps keeping lines short.

Your FACTORS variable is only needed for the order, since it is just the keys of the dictionary. Since Python 3.7 the order of dictionaries is guaranteed to be insertion order (implemented since CPython 3.6), so you also don't need it for the order if you are using a modern version of Python.

You have a spelling error in raindrops (but at least it is also present when calling it).

The convert function can be a bit simplified by using or.

SOUNDS = {3: "Pling", 5: "Plang", 7: "Plong"}

def raindrops(n):
    return [sound for d, sound in SOUNDS.items() if n % d == 0]

def convert(n):
    return "".join(raindrops(n)) or str(n)

You could also get rid of the dictionary altogether and just use a list of tuples:

SOUNDS = [(3, "Pling"), (5, "Plang"), (7, "Plong")]

def convert(n):
    return "".join(sound for d, sound in SOUNDS if n % d == 0) or str(n)

Instead of having a lot of testcases, which takes a lot of manual typing to setup, group similar testcases together and use unittest.TestCase.subTest:

class RaindropsTest(unittest.TestCase):
    def test_known_results(self):
        test_cases = [(1, "1"), (3, "Pling"), ...]
        for n, expected in test_cases:
            with self.subTest(n=n, expected=expected)):
                self.assertEqual(convert(n), expected)

For successful test cases this reports as one test case, but if it fails the additional information passed as keyword arguments is shown (here purposefully broken by supplying the wrong expected output):

======================================================================
FAIL: test_known_results (__main__.RaindropsTest) (expected='5', n=5)
----------------------------------------------------------------------
Traceback (most recent call last):
  File "/tmp/test.py", line 15, in test_known_results
    self.assertEqual(convert(n), expected)
AssertionError: 'Plang' != '5'
- Plang
+ 5


----------------------------------------------------------------------
Ran 1 test in 0.000s

FAILED (failures=1)