0

I have 4 variables, some of them will be True and False, and for each combinations, i will have to call one or few functions. i am currently using a if else statement for each case and i would like to know if there is a nicer way to get the same result with a dictionary or something else.

Thank you

here is my code :

 if (self.cpe_ip and self.cpe_passwd) and self.phone and self.pppoe: print "launch ISP portal, modem and radius" if self.isp() == "east": self.launchbell() else: self.launchtelus() print 'check modem...' self.modemstatus() radius = sgp_radius.Radius(self.pppoe) print 'check radius logs...' self.data = radius.sgp() self.radius_save() #exit(0) elif (self.cpe_ip and self.cpe_passwd) and not self.phone and not self.pppoe: print "launch modem test only" self.modemstatus() #exit(0) elif not(self.cpe_ip and self.cpe_passwd) and self.phone and not self.pppoe: #print "only Bell portal" if self.isp() == "east": self.launchbell() else: self.launchtelus() elif (self.cpe_ip and self.cpe_passwd) and not self.phone and self.pppoe: #print "launch modem and radius test." self.modemstatus() radius = sgp_radius.Radius(self.pppoe) print 'check radius logs...' self.data = radius.sgp() self.radius_save() elif not(self.cpe_ip and self.cpe_passwd) and not self.phone and self.pppoe: #print "only radius tests." radius = sgp_radius.Radius(self.pppoe) self.data = radius.sgp() self.radius_save() elif not(self.cpe_ip and self.cpe_passwd) and self.phone and self.pppoe: print "bell and radius tests." if self.isp() == "east": self.launchbell() else: self.launchtelus() radius = sgp_radius.Radius(self.pppoe) print 'check radius logs...' self.data = radius.sgp() self.radius_save() elif (self.cpe_ip and self.cpe_passwd) and self.phone and not self.pppoe: #print "launch modem and bell tests." if self.isp() == "east": self.launchbell() else: self.launchtelus() self.modemstatus() else: #print "test bell only" #launchbell(self.phone) exit(0)
Improve this question

2 Answers 2

Reset to default
5

One way to simplify it would be to recognise and factor out the duplication:

if self.phone: if self.isp() == "east": self.launchbell() else: self.launchtelus() if self.cpe_ip and self.cpe_passwd: print 'check modem...' self.modemstatus() if self.pppoe: radius = sgp_radius.Radius(self.pppoe) print 'check radius logs...' self.data = radius.sgp() self.radius_save()
Improve this answer
Sign up to request clarification or add additional context in comments.

5 Comments

ok thks for your quick answer, it means that i will have to use a for loop to test every element and launch the function if one of them is True...
@neonet I don't quite follow your comment. These three separate if clauses don't need to be looped over, you can put them as I show them; one after another in your (I'm guessing) test function.
What if he wants to print the currently-commented out strings depending on the different combinations? (Or, more generally, if tomorrow the logic changes and there is less duplication)
@GreenAsJade I would probably handle the strings separately, if really needed, before reaching if self.phone:. If the logic changes fundamentally (i.e. not just toggling three independent tests) then a completely different approach would be required, but that's hard to answer in the general case.
Yeah - if the final application is truly as simple as your simplification shows, hard to see why you would do anything else than what you did :)
2

You implied in your question that some sort of lookup table for the logic might be the way to go, and there are many ways you could make something like that.

One way would be, as you say, with a dict, as shown below.

This way results in rather more complex looking code than the nice looking "simplify and factor out duplication" solution for your particular example, but this method extends more easily to the case where you have more different logic for the different cases... I'm not saying it's "better" ... just a different option! 

def launch_isp_portal_and_radius(): print "launch ISP portal, modem and radius" # etc # etc DecisionTable = { # cpe_ip cpe_passwd phone ppoe (True, True, True, True ) : launch_isp_portal_and_radius, (True, True, False, False) : launch_modem_test_only, (False, False, True, False) : only_Bell_portal, (False, True, True, False) : only_Bell_portal, (True, False, True, False) : only_Bell_portal, (True, True, False, True ) : launch_modem_and_radius_test, (False, False, False, False) : only_radius_tests, (False, True, False, False) : only_radius_tests, (True, False, False, False) : only_radius_tests, (False, False, True, True ) : bell_and_radius_tests (False, True, True, True ) : bell_and_radius_tests, (True, False, True, True ) : bell_and_radius_tests, (True, True, True, False) : launch_modem_and_bell_tests } action = DecisionTable.get((self.cpe_ip, self.cpe_passwd, self.phone, self.ppoe)) if action: action() else: raise StandardError("Whoa, internal logic error!")

(And of course there are other ways to make a decision table - an array is another obvious option. The problem with an array is making the static declaration of it look readable for the logic it represents...)

(Edit: made the dict indexed on tuple, as per jons suggestion)

Improve this answer

5 Comments

This is actually the other approach I had in my head! Two comments: 1. a four-tuple of booleans would probably be a better key (inputs = tuple(map(bool, (cpe_ip, cpe_passwd, phone, ppoe)))); and 2. I would split the sub-tests (e.g. launch ISP) into sub-functions. I would also be tempted to use try: DecisionTable[inputs](); except KeyError: ....
Yeah - totally agree about factoring out common subfunctions. But OH MY GOD ... I had no idea you can index a dict with a tuple! That is heaps more elegant ... I will rewrite thusly!
Anything that can be hashed and equality-compared (inlcuding custom classes that implement __hash__ and __eq__) can be a key. Note that you don't define choices, so your last line is a NameError.
Thank you for all your comments, i like the way with the dictionary GreenAsJade, i was wondering how to implement it with a dictionary... and the Jons's way seems very elegant i have to try that out.
@jonrsharpe damn no regression test suite ;) :D neonet: the standard way of saying that you like a solution is to upvote it ;)

Start asking to get answers

Find the answer to your question by asking.

Ask question

Explore related questions

See similar questions with these tags.