2
\$\begingroup\$

I made a program, but I not happy with the quantity of code. The end result is good, but I believe it can be made much easier, only I don't know how.

The functionality: If equal items > 1 in list, then assign all equal items a unique set number. Below I made a unit test. I'm not happy with the class CreatSet. Can somebody advise me on how this can be implemented better?

import unittest


class Curtain(object):
  def __init__(self, type, fabric, number):
      self.type = type
      self.fabric = fabric
      self.number = number
      self.set_number = None

  def __str__(self):
      return '%s %s %s %s' % (self.number, self.type, self.fabric, self.set_name)

  def __eq__(self, other):
      return self.type == other.type and self.fabric == other.fabric

class CreatSet(object):
  def make_unique(self, original_list):
      checked = []
      for e in original_list:
          # If curtain: type and fabric is equal
          if e not in checked:
              checked.append(e)
      return checked

  def create_set(self, curtains):
      # Uniuqe items in list
      unique_list = self.make_unique(curtains)
      result = []
      for x in unique_list:
          # Create set list
          set_range = []
          for y in curtains:
              if y == x:
                  set_range.append(y)
          # Add set range into list
          result.append(set_range)

      # Create set number
      set_result = []
      set_number = 0
      for x in result:
          if len(x) == 1:
              set_result.append(x[0])
          else:
              set_number += 1
              for y in x:
                  y.set_number = set_number
                  set_result.append(y)
      # Return list ordered by number
      return sorted(set_result, key=lambda curtain: curtain.number)

class TestCreateSet(unittest.TestCase):
    def setUp(self):
      self.curtains = []
      self.curtains.append(Curtain('pleatcurtain', 'pattern', 0))
      self.curtains.append(Curtain('pleatcurtain', 'plain', 1))
      self.curtains.append(Curtain('pleatcurtain', 'pattern', 2))
      self.curtains.append(Curtain('foldcurtain', 'pattern', 3))
      self.curtains.append(Curtain('pleatcurtain', 'plain', 4))
      self.curtains.append(Curtain('foldcurtain', 'plain', 5))
      self.curtains.append(Curtain('pleatcurtain', 'pattern', 6))
      self.curtains.append(Curtain('foldcurtain', 'pattern', 7))

    def test_auto_set(self):
      creat_set = CreatSet()
      result = creat_set.create_set(self.curtains)
      # Creating set
      self.assertEqual(result[0].set_number, 1)  # pleatcurtain, pattern
      self.assertEqual(result[1].set_number, 2)  # pleatcurtain, plain
      self.assertEqual(result[2].set_number, 1)  # pleatcurtain, pattern
      self.assertEqual(result[3].set_number, 3)  # foldcurtain, pattern
      self.assertEqual(result[4].set_number, 2)  # pleatcurtain, plain
      self.assertEqual(result[5].set_number, None)  # foldcurtain, plain
      self.assertEqual(result[6].set_number, 1)  # pleatcurtain, pattern
      self.assertEqual(result[7].set_number, 3)  # foldcurtain, pattern

if __name__ == '__main__':
    unittest.main()
\$\endgroup\$
3
\$\begingroup\$

As you've already implemented Curtain.__eq__, if you also implement Curtain.__hash__ you can use it as a dictionary (or collections.Counter...) key, or a set member:

def __hash__(self):
    return hash(self.type) ^ hash(self.fabric)

Now make_unique is trivial:

def make_unique(self, original_list):
    return set(original_list)

(Note: if you require order to be retained, this will need additional work.)

This also allows you to easily determine how many of each distinct Curtain you have:

>>> from collections import Counter
>>> Counter(curtains)
Counter({<__main__.Curtain object at 0x02A15190>: 3, 
         <__main__.Curtain object at 0x031A0CF0>: 2, 
         <__main__.Curtain object at 0x031A0ED0>: 2, 
         <__main__.Curtain object at 0x0329EA30>: 1})

(Note: implementing Curtain.__repr__ would make this more readable!)


As pointed out in the comments, the __hash__ implementation I suggest has an issue - if either attribute type or fabric is changed, the hash will be different. You could protect these attributes by making them "read-only" using properties:

class Curtain(object):

    def __init__(self, type, ...):
        self._type = type # note leading underscore on attribute

    @property # defining setter but no getter
    def type(self):
        return self._type

Alternatively, you can implement Curtain.__lt__ etc., then sort the list and use itertools.groupby to get your groups of "equal" Curtains.


Either way, I would not implement CreatSet as a class, there's no need (as should be clear from the fact that there is no __init__ and no class or instance attributes). Just have one class, and two functions:

class Curtain(object):
    ...

def create_set(curtains):
    ...

def make_unique(curtains):
    ...

Your code is generally compliant with the style guide (well done!) but you could do with some explanatory docstrings.

| improve this answer | |
\$\endgroup\$
  • 1
    \$\begingroup\$ If __hash__ depends on mutable attributes, one must take care not to change those attributes while the objects are in a set or dict keys. I think that is begging for trouble. \$\endgroup\$ – Janne Karila Nov 11 '14 at 13:08
  • \$\begingroup\$ @JanneKarila that is a good point, I will edit to that effect \$\endgroup\$ – jonrsharpe Nov 11 '14 at 13:14
  • \$\begingroup\$ @Robert no problem. Note that if you make a Curtain hash-able, you can use Janne's answer using x as the key, rather than the tuple x.type, x.fabric. \$\endgroup\$ – jonrsharpe Nov 13 '14 at 10:43
2
\$\begingroup\$
  • Building the set_result list in create_set is not necessary, because the same objects are already in curtains. You can simply use

    return sorted(curtains, key=lambda curtain: curtain.number)
    
  • It might be better not to return any value from create_set. That would highlight the fact that it modifies the objects in place.

  • Here's my proposal for create_set that uses a defaultdict to accomplish both the detection of unique values and the grouping. However, your test will not pass because defaultdict is unordered, and therefore the numbers 1,2 and 3 can be assigned differently to the sets. If all you need are unique numbers, you could change the test not to expect an exact numbering. Note that this is a stand-alone function because there is no reason to put it in a class.

    def create_set(curtains):
        d = collections.defaultdict(list)
        for x in curtains:
            d[x.type, x.fabric].append(x)
    
        # Create set number
        set_number = 0
        for x in d.values():
            if len(x) > 1:
                set_number += 1
                for y in x:
                    y.set_number = set_number
    
| improve this answer | |
\$\endgroup\$
  • \$\begingroup\$ thanks for solution, this code is better, cleaner and nicer. I can't click twice on the button finding the solving my problem. Both feedback are great. \$\endgroup\$ – Robert Nov 13 '14 at 10:45

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.