Make a dict class from scratch in Python

Here's the API my teacher assigned:

and here is what I came up with:

class MyMap:
    def __init__(self,size=10):
        self.size = size
        self.internal_list = [None] * size
    def _getHash(string,size):
        total = 0
        for ch in string:
            total += ord(ch)
        total = total % size
        return total
    def set(self,key,value):
        listnum = MyMap._getHash(key,self.size)
        if key in self.internal_list:
            print("already there")
            return None
        if self.internal_list[listnum] == None or self.internal_list[listnum] is type(tuple):
            self.internal_list[listnum] = (key, value)
        else:
            pass
    def get(self,key):
        listnum = MyMap._getHash(key,self.size)
        if self.internal_list[listnum] == None:
            return None
        else:
            return self.internal_list[listnum][2]
    def size(self):
        sizecount = 0
        count = 0
        while len(self.internal_list)-count != 0:
            if self.internal_list[count] != None:
                sizecount += 1
            count += 1 
        return sizecount
    def getKeys(self):
        keylist = []
        count = 0
        while len(self.internal_list) - count != 0:
            if self.internal_list[count] != None:
                keylist.append(self.internal_list[count][0])
            count += 1
        return keylist

Just wondering

• if it will fulfill the requirements of the assignment

• what I can do to clean up the code

Here's what I've tested:

x = MyMap()
x.set('test',2)
x.set('crest',3)
x.set('best',5)
print(MyMap.size(x))
print(x.get('test'))
print(MyMap.getKeys(x))
print(x.internal_list)

and the results...

The first thing you need to do is learn how to unittest your code. This will assume Python3.

# filename: test_MyMap.py
import unittest
import unittest.mock as mock
from mymap import MyMap # the filename for your code

class Test(unittest.TestCase):

    # here's where you write the tests.
    def test_default_size(self):
        m = MyMap()
        self.assertEqual(m.size, 10)

    def test_set_method(self):
        """we use unittest.mock.patch to swap in an easy version of
        _getHash that we know cannot fail. This way we don't introduce
        any other reason of failure into our test for MyMap.set"""

        def replace_getHash(self,key):
            return {'string':0, 1:1, tuple(1,2):2}[key]
        with mock.patch("__main__.MyMap._getHash", new=replace_getHash):
            for key,value in [('string','value'),
                              (1,2),
                              (tuple(1,2), tuple(3,4))]:
                m.set(key, value)
                self.assertEqual(m.internal_list[m._getHash(key)], value)

    def test_get_method(self):
        # etc for all the specs you need to fulfill

if __name__ == "__main__":
    """Will run all the TestCase classes, which each run all their methods
    that begin with test_"""
    unittest.main()

The first thing you should do when you get an assignment or any other spec to write from is design a set of unit tests (as above) to write your code to meet. It's much harder to create unit tests from a program than from a spec so do it this way first! Then every time you change the code, check it against the unit tests to make sure you haven't broken anything.

As far as the actual review goes, it's going to be hard to look at until you can design a full suite of tests against it. A quick glance looks like this will fail if you give it a non-iterable key (e.g. an int, which should be doable) and will also lead to hash collision if you give it an anagram (e.g. foo.set("test",3) == foo.set("etst", 3)). These are edge cases that your unit tests should catch.

Beyond that I'd implement item access (so you can do my_map_object[some_key] instead of having to rely on my_map_object.get(some_key)) and it really seems like you could clean up your MyMap.getkeys function. Something like:

def getkeys(self):
    return [keyvalue[0] for keyvalue in self.internal_list if keyvalue is not None]

should work perfectly. It also stands to reason that

def size(self):
    return sum([1 for key in self.getkeys()])

would work too!

Oh and your MyMap._getHash function will break completely, since string will never be iterable. It should be def _getHash(self, string) (and reference self.size()) rather than def _getHash(string, size) unless you make it a @staticmethod (which WOULD make sense except that your size modulo is dependent on your particular mapping!)

StackExchange Code Review Q#69437, answer score: 3