Solution to basic unit tests

Question

I'm working my way through an intermediate Python course on Pluralsight that focuses on unit testing and just finished my first assignment.

Basically the course itself focuses on teaching you how to write correct test-cases and gives you a baseline of the class to start solving the tests by yourself.

To clarify:

I'm more concerned on the implemented solutions to the test-cases than on the test-cases themselves. Sorry for any confusion!

---

I was hoping that I could get some feedback on my implemented solutions to the test-cases (they all are passing):

import unittest

from phonebook import Phonebook

class PhoneBookTest(unittest.TestCase):

    def setUp(self):
        self.phonebook = Phonebook()

    def test_phonebook_with_normal_entries_is_consitent(self):
        self.phonebook.add("Bob", "12345")
        self.phonebook.add("Mary", "012345")
        self.assertTrue(self.phonebook.is_consistent())

    def test_phonebook_with_duplicate_entries_is_inconsitent(self):
        self.phonebook.add("Bob", "12345")
        self.phonebook.add("Sue", "12345")  # identical to Bob
        self.assertFalse(self.phonebook.is_consistent())

    def test_phonebook_with_same_prefix_is_inconsistent(self):
        self.phonebook.add("Bob", "12345")
        self.phonebook.add("Sue", "123")  # prefix of Bob
        self.assertFalse(self.phonebook.is_consistent())

---

Here is my actual Phonebook class:

class Phonebook:

    def __init__(self):
        self._entries = {}

    def add(self, name, number):
        self._entries[name] = number

    def lookup(self, name):
        return self._entries[name]

    def is_consistent(self):
        if not self._entries:
            return True
        else:
            if self.check_duplicate():
                if self.check_prefix():
                    return True
        return False

    def check_duplicate(self):
        seen = set()
        for x in self.get_values():
            if x not in seen:
                seen.add(x)
            else:
                return False
        return True

    def check_prefix(self):
        prefixes = []
        for contact_number in self.get_values():
            if contact_number not in map(lambda number: number[0:len(contact_number)], prefixes):
                prefixes.append(contact_number)
            else:
                return False
        return True

    def get_values(self):
        return self._entries.values()

I'm mostly concerned of refactoring and the check_function and check_prefix functions performance and overall implementation. However I still consider myself to be a beginner, so all help would be appreciated! :)

Any feedback would be great!

Answer 1

• Bug: The result of check_prefix depends on the order of values. Basically, it compares each pair of values once, but truncates only one of the two for comparison. On my Python 3.5 your test fails occasionally.
• Duplicate values are also prefixes of one another. It is not necessary for is_consistent to check for both.

• It is not necessary to handle the empty phonebook as a special case in is_consistent as the check functions return True swiftly anyway in that case. Thus the function can be simplified to

  def is_consistent(self):
      return self.check_prefix()
  

• A more efficient solution for check_prefix would be to sort the values and compare adjacent ones. Making use of pairwise from Itertool recipes, and the fact that the prefix will sort first:

  def check_prefix(self):
      return not any(b.startswith(a) 
                     for a, b in pairwise(sorted(self.get_values())))
  

Answer 2

Tackling unit tests can be a grueling task, and depending on how thorough you want to do it can generate a lot of cases. My general principle is to test, at least, all the public functions, and for each function make sure the tests enters into the various blocks and conditions of the function I'm testing.

The test cases you've provided are only testing is_consistent, which subsequently tests the check_duplicate and check_prefix. But there are edge cases which you aren't testing. Like the not self._entries case. A point can be made, as is in the other answer, that this is an unneeded case, but as long as it is present in your code I would write a test for it.

If I were to write test cases for this code I would most likely end up with something following this pattern:

• One test class for each function I would like to test:
  • Phonebook__is_consistent
  • Phonebook__add
  • Phonebook__lookup
  • and so on...
• For each class, I would add enough cases to cover normal and edge situations, for example for is_consistent:
  • test_Empty__True
  • test_Normal_five_element__True
  • test_Two_duplicates__False
  • test_With_prefixes__True

I do break standard naming conventions here, to add a little more readability to my names. In the class names I use double underscores to separate class name from method name. And in the test names I use double underscores to separate input conditions vs expected output result.

Usually I would also run my unittests with increased verbosity so that I'm able to see all the succeeding tests as well as the failing. This could be a matter of personal preferences though.

Comments to implementation

• No test for equal names – If I add "Bob" for the second time, it simply overwrites the first entry. Is that intentional? I would rather have an error message in this case, and provide an update method in addition to the add method.

• Alternate check_duplicate – You manually build a set, and check whether you've seen stuff there. Note that Python can generate a set based on upon lists, and since sets are unique by nature, this method could have been written as:

  def check_duplicate(self):
      return len(self._entries) != len(set(self._entries))
  

  In other words, if there are duplicates there would not be the same number of entries in the list as in the set based upon the list.

• Try to avoid the if condition return True pattern – This pattern can often be rewritten as in previous case to be return condition. No need for the explicit return True or return False, when those are the only options.

  If you kept all tests from is_consistent(), which aren't needed as described in other answers, this could have been written as:

  def is_consistent():
      return not self._entries or 
            (self.checkDuplicate() and self.check_prefix())
  

• Let the names of boolean functions reflect result – checkDuplicate and check_prefix are inconsistently named regarding camelCase and snake_case and don't indicate anything on result. Better names would be has_duplicates and has_common_prefixes.

• Better handling if a name is not present – If I try to lookup a name not in the phonebook, the program would crash. It would be nicer, if it either raised an exception allowing the caller to handle it, or to return some other value to allow the caller to continue without a crash.

• Some additional features which would be nice to have:

  • No possibility to removed entries – For a proper phonebook, I would also like to have the possibility to remove someone, not only add people.
  • Getting a count of entries? – Always nice to know how many friends you have.
  • Printing the entries? – It would be nice to either make a printed version of the phonebook, and/or to skim through the entire list visually. It would help locate "Bob", or was it "Robert" or "Bobby" (which all could refer to the same person).
  • Ability to check who owns a number – The reverse lookup could also be nice to have.
  • Persistent storage of phonebook – Lastly, a phonebook only existing while you are running the program is of limited use. So persistent storage to either file or database or the cloud, would be extremely nice.