1
\$\begingroup\$

I am trying to create a class that parses a JSON and selects all labels, or just a particular label that you prefer to later do a calculation with the value pair extracted off that label. This is working well for .all() This is the first function, wich selects ALL available labels. But then I want to make it flexible and be able to call names = IncomeSources(result).spa() to see the revenue results for the spa only. The code is working fine, but I will die if I have to write a function for each label separately. Is there more efficient way to do it ?

class IncomeSources(object):

    def __init__(self, result):
        self.result = result

    def all(self):
        labels_list = []
        for index in self.result:
            for key, value in index.items():
                if 'accountLabelType' in key:
                    names = value
                    labels_list.append(names)
                    collection = set(labels_list)

        return collection

    def brickyard(self):
        name = 'Brickyard'
        labels_list = []
        for index in self.result:
            for key, value in index.items():
                if 'accountLabelType' in key and value.startswith('BYD'):
                    names = value
                    labels_list.append(names)
                    collection = set(labels_list)
        return collection

    def schoolhouse(self):
        name = 'Schoolhouse'
        labels_list = []
        for index in self.result:
            for key, value in index.items():
                if 'accountLabelType' in key and value.startswith('SCH'):
                    names = value
                    labels_list.append(names)
                    collection = set(labels_list)
        return collection

    def spa(self):
        name = 'Spa'
        labels_list = []
        for index in self.result:
            for key, value in index.items():
                if 'accountLabelType' in key and value.startswith('Spa'):
                    names = value
                    labels_list.append(names)
                    collection = set(labels_list)
        return collection
\$\endgroup\$
1
  • \$\begingroup\$ Titles for Code Review questions should describe the purpose of the code, not your concern about the code. See How to Ask. \$\endgroup\$ – 200_success Feb 18 '16 at 9:40
1
\$\begingroup\$

Duplicated code

In order to remove duplicated code, the key is to identify the parts that are identical/different from one place to another. In your case, you could add a filter parameter to your all function (which would then deserve to be named differently).

You could write something like :

def get_result(self, beginFilter=None):
    labels_list = []
    for index in self.result:
        for key, value in index.items():
            if 'accountLabelType' in key and (beginFilter is None or value.startswith(beginFilter)):
                names = value
                labels_list.append(names)
                collection = set(labels_list)
    return collection

def brickyard(self):
    return self.get_result('BYD')

def schoolhouse(self):
    return self.get_result('SCH')

def spa(self):
    return self.get_result('Spa')

As a side-note, I had not given enough thought before writing the code above but if I had to re-do it from the beginning, I'd choose '' as a default value because it wouldn't need a special handling (all strings start with '' as far as I can tell).

Simplifying code

All your name variables were useless. Your names variable is not really useful neither. Also, labels_list could be named labels.

Finally, your conversion from list of labels to set of labels could be done only once at the very end of the function.

def get_result(self, beginFilter=None):
    labels = [] 
    for index in self.result:
        for key, value in index.items():
            if 'accountLabelType' in key and (beginFilter is None or value.startswith(beginFilter)):
                labels.append(value)
    return set(labels) 

Now, it looks clearer that labels could be a set directly instead of converting it afterward.

def get_result(self, beginFilter=None):
    labels = set()
    for index in self.result:
        for key, value in index.items():
            if 'accountLabelType' in key and (beginFilter is None or value.startswith(beginFilter)):
                labels.add(value)
    return labels

You could also write this as a single set comprehension. Set comprehension is a convenient way to define set (you also have list comprehension and dict comprehension and even something called generator expression but this is out of scope at the moment).

It usually goes like this

my_list = [func(x) for x in my_iterable]  # list comprehension
my_set = {func(x) for x in my_iterable}  # set comprehension
my_dict = { my_key(x): my_val(x) for x in my_iterable}  # dict comprehension

In all 3 cases, one could add an optional filtering part so you could write something like:

my_set = {func(x) for x in my_iterable if some_property(x)}

Finally, you can also add more nested iterations. In your case, we could write (but some people would consider that this is going too far):

def get_result(self, beginFilter=None):
    return { value
        for index in self.result
        for key, value in index.items()
        if 'accountLabelType' in key and (beginFilter is None or value.startswith(beginFilter))}

Back to where we came from

Now that I understand better the point of your code, we can try to clean up the get_result function. The filtering can actually be moved out of the function.

def all(self):
    return { value
        for index in self.result
        for key, value in index.items()
        if 'accountLabelType' in key}

def brickyard(self):
    return {v for v in self.all() if v.startswith('BYD')}

def schoolhouse(self):
    return {v for v in self.all() if v.startswith('SCH')}

def spa(self):
    return {v for v in self.all() if v.startswith('Spa')}

or with a helper function :

def all(self):
    return { value
        for index in self.result
        for key, value in index.items()
        if 'accountLabelType' in key}

def filter(self, begin):
    return {v for v in self.all() if v.startwith(begin)}

def brickyard(self):
    return self.filter('BYD')

def schoolhouse(self):
    return self.filter('SCH')

def spa(self):
    return self.filter('Spa')

More

In order not to call all multiple times, one could imagine saving the result.

\$\endgroup\$
5
  • \$\begingroup\$ Thank you for the thorough review. This is really helpful ! \$\endgroup\$ – xavier Feb 19 '16 at 3:39
  • \$\begingroup\$ By the way, I have never seen using curly braces on functions before. Can you give me a short explanation of why and how are you using them , or point me to a documentation ? I thought curly braces where only used on dicts inPython. \$\endgroup\$ – xavier Feb 19 '16 at 6:17
  • \$\begingroup\$ I understand now the curly braces, but I don't get why def all(self): return { value for index in self.result for key, value in index.items() if 'accountLabelType' in key} returns a set ? I thought it would just return a list at most \$\endgroup\$ – xavier Feb 19 '16 at 6:36
  • \$\begingroup\$ Hi! I'm glad you liked the answer. I'll add explanations and other improvements in the next hours. \$\endgroup\$ – SylvainD Feb 19 '16 at 7:45
  • 1
    \$\begingroup\$ No problem! I've updated my answer, let me know if you need more clarification. \$\endgroup\$ – SylvainD Feb 19 '16 at 9:16

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.