0
\$\begingroup\$

In the code below, all pairs of functions are very similar:

  1. set function

    • rankListId is different
    • rankListName is different
    • class of Mongoengine is different
  2. get function

    • the rankListId is different

How can I make these function into just one function?

def set_male_rank_list(self):
    rankList              = RankList()
    rankList.rankListId   = "zhineng_male_rank_list"
    rankList.rankListName = u"十大男生比例最高职业"
    rankList.itemList     = []

    charts = ZhinengMaleCharts.objects.all()[:10]

    for c in charts:
        rankList.itemList.append(RankListItem(c.zhineng_id, c.zhineng_id))

    self.rankListClient.setRankList("", rankList)

    return rankList

def get_male_rank_list(self):
    return self.rankListClient.getRankList("", "zhineng_male_rank_list")

def set_female_rank_list(self):
    rankList              = RankList()
    rankList.rankListId   = "zhineng_female_rank_list"
    rankList.rankListName = u"十大女生比例最高职业"
    rankList.itemList     = []

    charts = ZhinengFemaleCharts.objects.all()[:10]

    for c in charts:
        rankList.itemList.append(RankListItem(c.zhineng_id, c.zhineng_id))

    self.rankListClient.setRankList("", rankList)

    return rankList

def get_female_rank_list(self):
    return self.rankListClient.getRankList("", "zhineng_female_rank_list")

def set_salary0_rank_list(self):
    rankList              = RankList()
    rankList.rankListId   = "zhineng_salary0_rank_list"
    rankList.rankListName = u"十大毕业生高薪职业"
    rankList.itemList     = []

    charts = ZhinengSalary0Charts.objects.all()[:10]

    for c in charts:
        rankList.itemList.append(RankListItem(c.zhineng_id, c.zhineng_id))

    self.rankListClient.setRankList("", rankList)

    return rankList

def get_salary0_rank_list(self):
    return self.rankListClient.getRankList("", "zhineng_salary0_rank_list")

def set_salary5_rank_list(self):
    rankList              = RankList()
    rankList.rankListId   = "zhineng_salary5_rank_list"
    rankList.rankListName = u"十大五年后高薪职业"
    rankList.itemList     = []

    charts = ZhinengSalary5Charts.objects.all()[:10]

    for c in charts:
        rankList.itemList.append(RankListItem(c.zhineng_id, c.zhineng_id))

    self.rankListClient.setRankList("", rankList)

    return rankList

def get_salary5_rank_list(self):
    return self.rankListClient.getRankList("", "zhineng_salary5_rank_list")

def set_salary5_inc_rank_list(self):
    rankList              = RankList()
    rankList.rankListId   = "zhineng_salary5_inc_rank_list"
    rankList.rankListName = u"十大五年涨幅最大职业"
    rankList.itemList     = []

    charts = ZhinengSalary5IncCharts.objects.all()[:10]

    for c in charts:
        rankList.itemList.append(RankListItem(c.zhineng_id, c.zhineng_id))

    self.rankListClient.setRankList("", rankList)

    return rankList

def get_salary5_inc_rank_list(self):
    return self.rankListClient.getRankList("", "zhineng_salary5_inc_rank_list")

def set_salary5_inc_ratio_rank_list(self):
    rankList              = RankList()
    rankList.rankListId   = "zhineng_salary5_inc_ratio_rank_list"
    rankList.rankListName = u"十大五年涨幅比例最大职业"
    rankList.itemList     = []

    charts = ZhinengSalary5IncRatioCharts.objects.all()[:10]

    for c in charts:
        rankList.itemList.append(RankListItem(c.zhineng_id, c.zhineng_id))

    self.rankListClient.setRankList("", rankList)

    return rankList

def get_salary5_inc_ratio_rank_list(self):
    return self.rankListClient.getRankList("", "zhineng_salary5_inc_ratio_rank_list")

def set_stability_asc_rank_list(self):
    rankList              = RankList()
    rankList.rankListId   = "zhineng_stability_asc_rank_list"
    rankList.rankListName = u"十大最不稳定职业"
    rankList.itemList     = []

    charts = ZhinengYearsDesc.objects.all()[:10]

    for c in charts:
        rankList.itemList.append(RankListItem(c.zhineng_id, c.zhineng_id))

    self.rankListClient.setRankList("", rankList)

    return rankList

def get_stability_asc_rank_list(self):
    return self.rankListClient.getRankList("", "zhineng_stability_asc_rank_list")

def set_stability_desc_rank_list(self):
    rankList              = RankList()
    rankList.rankListId   = "zhineng_stability_desc_rank_list"
    rankList.rankListName = u"十大最稳定职业"
    rankList.itemList     = []

    charts = ZhinengYearsAsc.objects.all()[:10]

    for c in charts:
        rankList.itemList.append(RankListItem(c.zhineng_id, c.zhineng_id))

    self.rankListClient.setRankList("", rankList)

    return rankList

def get_stability_desc_rank_list(self):
    return self.rankListClient.getRankList("", "zhineng_stability_desc_rank_list")

def set_major_ratio_asc_rank_list(self):
    rankList              = RankList()
    rankList.rankListId   = "zhineng_major_ratio_asc_rank_list"
    rankList.rankListName = u"十大就页面最窄职业"
    rankList.itemList     = []

    charts = ZhinengMajorsAsc.objects.all()[:10]

    for c in charts:
        rankList.itemList.append(RankListItem(c.zhineng_id, c.zhineng_id))

    self.rankListClient.setRankList("", rankList)

    return rankList

def get_major_ratio_asc_rank_list(self):
    return self.rankListClient.getRankList("", "zhineng_major_ratio_asc_rank_list")

def set_major_ratio_desc_rank_list(self):
    rankList              = RankList()
    rankList.rankListId   = "zhineng_major_ratio_desc_rank_list"
    rankList.rankListName = u"十大就页面最广职业"
    rankList.itemList     = []

    charts = ZhinengMajorsDesc.objects.all()[:10]

    for c in charts:
        rankList.itemList.append(RankListItem(c.zhineng_id, c.zhineng_id))

    self.rankListClient.setRankList("", rankList)

    return rankList

def get_major_ratio_desc_rank_list(self):
    return self.rankListClient.getRankList("", "zhineng_major_ratio_desc_rank_list")

def set_male_priority_rank_list(self):
    rankList              = RankList()
    rankList.rankListId   = "zhineng_male_priority_rank_list"
    rankList.rankListName = u"十大男性有优势专业"
    rankList.itemList     = []

    charts = ZhinengMaleMinusFemaleCharts.objects.all()[:10]

    for c in charts:
        rankList.itemList.append(RankListItem(c.zhineng_id, c.zhineng_id))

    self.rankListClient.setRankList("", rankList)

    return rankList

def get_male_priority_rank_list(self):
    return self.rankListClient.getRankList("", "zhineng_male_priority_rank_list")

def set_female_priority_list(self):
    rankList              = RankList()
    rankList.rankListId   = "zhineng_male_priority_rank_list"
    rankList.rankListName = u"十大女性有优势专业"
    rankList.itemList     = []

    charts = ZhinengFemaleMinusMaleCharts.objects.all()[:10]

    for c in charts:
        rankList.itemList.append(RankListItem(c.zhineng_id, c.zhineng_id))

    self.rankListClient.setRankList("", rankList)

    return rankList

def get_female_priority_rank_list(self):
    return self.rankListClient.getRankList("", "zhineng_female_priority_rank_list")
\$\endgroup\$
  • \$\begingroup\$ What is the name of the class that we are looking at? And where do these things ZhinengMaleMinusFemaleCharts come from? \$\endgroup\$ – 200_success May 20 '15 at 17:31
  • \$\begingroup\$ these are the schma of mongoengine, I do not find a good way to put so much code, so I ignore them, may be you can tell me a good way to do this. \$\endgroup\$ – roger May 25 '15 at 3:37
1
\$\begingroup\$

I think there are more issues here than just these methods you've pasted. I'll first address the overall idea here and then discuss changes you can make without changing the interface.

Data driven design

You have several methods here, each only different in their parametrisation, not their actual behaviour. This is a huge red flag and should be addressed immediately.

Now how you do that is another thing. Below I'll refactor it into a more functional approach and under the assumption that the interface can't change because of external code not under your control.

However I'd also like to suggest another idea here, that might, or might not be viable for you.

Since the (not visible) container class has so many methods it doesn't look very nice, the names are repeated all over the place and the end result are horrible names. How about rather than having several get/set_*_rank_list methods, you'd expose wrapper objects with two methods instead. So e.g.

class Foo(object):
    def __init__(self, ...):
        ...
        self.male_rank_list = RankListWrapper(
            "zhineng_male_rank_list",
            u"十大男生比例最高职业",
            ZhinengMaleCharts,
            self.rank_list_client
        )
        ...

Then the access pattern would look like:

foo.male_rank_list.fetch() # instead of foo.get_male_rank_list()
foo.male_rank_list.update() # instead of foo.set_male_rank_list()

Not the best method names, but still better than get/set.

What you get here is a bit more flexibility and a lot of typing reduction. This could of course also be achieved with more meta-programming, but IMO this is good enough for what I can gather from your code as what your requirements would be.

Stylistic changes

  • Follow PEP8. Meaning the instance variables should have names in lower case with underscores as separators.
  • Why are the RankLists not initialised with their constructor? If you can't change that, then you could create a wrapper function like create_rank_list to address that.
  • The for c in charts loop is verbose, the same thing can be more easily achieved with a list comprehension:

    rankList.itemList = [RankListItem(c.zhineng_id, c.zhineng_id)
                         for c in charts]
    

    The strange thing here is that RankListItem takes the ID parameter twice. If possible change that to default to the first parameter. Otherwise you could create a helper function like create_rank_list_item(id).

At this point you could either go the functional, or the more data driven route - I opted for the latter in order to more closely mimic the idea described above. By putting all the parameters for the functions into separate definitions (be it classes, dictionaries, plain, or named tuples) the code gets much shorter and reusable.

The end result looks something like this:

def create_rank_list(id, name, item_list):
    result = RankList()
    result.rankListId = id
    result.rankListName = name
    result.itemList = item_list
    return result

def create_rank_list_item(id):
    return RankListItem(id, id)

class RankListDefinition(object):
    def __init__(self, id, name, charts):
        self.id = id
        self.name = name
        self.charts = charts

MALE_RANK_LIST = RankListDefinition(
    "zhineng_male_rank_list",
    u"十大男生比例最高职业",
    ZhinengMaleCharts
)
...

class Foo(object):
    ...
    def set_rank_list(self, definition):
        charts = definition.charts.objects.all()[:10]

        rank_list = create_rank_list(
            definition.id,
            definition.name,
            [create_rank_list_item(c.zhineng_id) for c in charts]
        )

        self.rankListClient.setRankList("", rank_list)

        return rank_list

    def get_rank_list(self, definition):
        return self.rankListClient.getRankList("", definition.id)

    def set_male_rank_list(self):
        return self.set_rank_list(MALE_RANK_LIST)

    def get_male_rank_list(self):
        return self.get_rank_list(MALE_RANK_LIST)
    ...

As you can see everything is factored into code (get/set_rank_list) and data (MALE_RANK_LIST, ...), so by adding one more definition (and two methods in this variant) you're done with adding a new list.

Since the wrapper methods are pretty useless I'd still suggest moving to the indirect approach mentioned above.

Also, if all of these methods were called one after another, I'd also remove all of them in favour of a loop over a list of "rank list definitions" instead (just saying).

\$\endgroup\$
  • \$\begingroup\$ thanks very much.I prefer using fetch/update, but what is self.rank_list_client? And I takes the ID parameter twice is because most of time the first time is id, the second is name, but this time I have no id, so I set id the same as name. \$\endgroup\$ – roger May 21 '15 at 2:56
  • \$\begingroup\$ sorry, I think I know what self.rank_list_client is, yours is better \$\endgroup\$ – roger May 21 '15 at 3:11
0
\$\begingroup\$

I'm a bit rusty at Python so my syntax might be a bit wrong, but something like this should work...

The point of methods is to avoid redundancy. The logic is exactly the same in both of the functions above with a few small exceptions, so why not use ifs and string manipulations for them?

def set_rank_list(self, gender):
    rankList              = RankList()
    rankList.itemList     = []
    rankList.rankListId   = "zhineng_%s_rank_list" % gender
    rankList.rankListName = u"十大男生比例最高职业"
    if gender is 'male':
        charts = ZhinengMaleCharts.objects.all()[:10]
    else:
        charts = ZhinengFemaleCharts.objects.all()[:10]

    for c in charts:
        rankList.itemList.append(RankListItem(c.zhineng_id, c.zhineng_id))

    self.rankListClient.setRankList("", rankList)

    return rankList

# and you can add these for convenience
def set_female_rank_list(self):
    return self.set_rank_list('female')

def set_male_rank_list(self):
    return self.set_rank_list('male')
\$\endgroup\$
0
\$\begingroup\$

Just extending Chen's solution

rankList = RankList()
rankList.rankListId ="zhineng_male_rank_list"
rankList.rankListName = u"十大男生比例最高职业"
rankList.itemList = []

# makes it easier sex is later decided to be presented as 'M'/'F'
@classmethod
def is_male(gender)
   return {
      'male': True,
      'female': False,
    }.get(x, False)  # can be replaced by None for a wrong gender


self.chartSize = 10

def set_rank_list(self,gender):
    charts = ZhinengMaleCharts if is_male(gender) else ZhinengFemaleCharts
    charts = charts[0:self.chartSize]
    rankList.itemList = [RankListItem(c.zhineng_id, c.zhineng_id) for c in charts] 
    self.rankListClient.setRankList("", rankList)
    return rankList

Any comments on the following changes is appreciated.

\$\endgroup\$

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.