Removing redundant lines from script

Question

Context: I've been learning Python for a few months and I'm able to write codes which do work, but they normally look very ugly and contain a lot of unnecessary code. But I normally struggle to find better ways to do things because of my limited knowledge.

I would like some advice on what I can improve on the code below. FYI - it works perfectly well, but I'm guessing there must be ways to improve it. I know I'm repeating myself in the loops, for example.

def getTableGroupTopMember_Test1(coordinates):
 '''
 Depending on the number of coordinates given, this function/code returns a unique coordinate. 
 '''
 mylist = coordinates
 for adc in activeDataConnections:
 if mylist.Count == 1:
 for table in adc:
 if table.Name == mylist[0]:
 print "/"
 elif mylist.Count == 2:
 for table in adc:
 if table.Name == mylist[0]:
 for topgroup in table.TopAxis.Groups:
 if topgroup.Name == mylist[1]:
 print topgroup.Address
 elif mylist.Count == 3:
 for table in adc:
 if table.Name == mylist[0]:
 for topgroup in table.TopAxis.Groups:
 if topgroup.Name == mylist[1]:
 for topmember in topgroup:
 if topmember.Name == mylist[2]:
 print topmember.Address
 else:
 print "your code is shit!"
getTableGroupTopMember_Test1(["Table10"])
getTableGroupTopMember_Test1(["Table10", "profile_julesage",])
getTableGroupTopMember_Test1(["Table10", "profile_julesage", "_25_to_34"])

Output:

/
/2
/2[3]

• theodox: What is the API you're working with? Or to put it another way, what does activeDataConnections contain? Most database modules (eg sqllite) include query functionality which would eliminate most of the repetitive loop based code

Answer 1

This is what I would do.

First, rename coordinates to coord_list so you can eliminate mylist which is kind of useless since you only use immediately assign it to coordinates. To make things slightly simpler I assigned count to coord_list.Count so now you can just check count instead of coord_list.Count. Instead of executing the loop if the Count is bad, we check that count is between 1 and 3 before entering the loop. From here it loops though the activeDataConnections and has been changed so it checks count instead of repetition of the same logic.

Here is the code:

def getTableGroupTopMember_Test1(coord_list):
 '''
 Depending on the number of coordinates given, this function/code returns a unique coordinate. 
 '''
 count = coord_list.Count
 # thanks to 200_success
 if not 0 < count < 4:
 print "your code is shit!"
 return
 for adc in activeDataConnections:
 for table in adc:
 if table.Name == coord_list[0] and count == 1:
 print "/"
 elif table.Name == coord_list[0]:
 for topgroup in table.TopAxis.Groups:
 if topgroup.Name == coord_list[1] and count == 2:
 print topgroup.Address
 elif topgroup.Name == coord_list[1]:
 for topmember in topgroup:
 if topmember.Name == coord_list[2]:
 print topmember.Address
getTableGroupTopMember_Test1(["Table10"])
getTableGroupTopMember_Test1(["Table10", "profile_julesage",])
getTableGroupTopMember_Test1(["Table10", "profile_julesage", "_25_to_34"])

• 200_success: Your indentation is off.
• 200_success: Python supports double-ended comparisons. Also, I'd suggest reversing the condition and returning early. if not 1 <= count < 4: print "blah"; return;
• Dom: That should be it.
• Boosted_d16: I can now see how I could have improved my code. Thanks for the effort.

Answer 2

This could be even a little shorter if you use list comprehension, yields and iteration to avoid all the if-tests (and, btw, use named variables instead of indices for clarity!)

def extract_all_coords (tablename, groupname, membername):
 valid_tables = [t for t in activeDataConnections.tables if t.Name == tablename]
 for t in tables:
 yield ('table', t)
 valid_groups = [g for g in t.TopAxis.Groups if g.Name == groupname]
 for g in valid_groups:
 yield ('group', g)
 valid_members =[m for m in g if m.Name == membername]
 for m in valid_members:
 yield ('member', m)
def extract_coords( tablename, groupname, membername):
 return dict([k for k in extract_all_coords(tablename, groupname, membername)])

In general I'd try to avoid functions that are open ended on both ends -- in your case, taking a variable number of inputs and returning a variable number of outputs. In this case, returning a dictionary means you should always get one answer to the question your asking and not have to write too much conditional logic to parse the answer later