python function compression

Question

I have written this big function to do some formatting in my python code. Would you be able to suggest anyways to make this smaller ?

def disfun(String1,String2,String3): if String3 == "A" or String3 == "B": if String3 == "A": pass elif String3 == "B": print "#"*54 print "##"," "*48,"##" print "##",'{0:^48}'.format(String2),"##" print "##",'{0:^48}'.format(String1),"##" print "##"," "*48,"##" print "#"*54 elif String3 == "C": print "-"*40 print "--",'{0:^34}'.format(String2),"--" print "-"*40 elif String3 == 'D': String2 = ' * '.join(String2) print "#"*54 print "##",'{0:^48}'.format(String2),"##" print "##",'{0:^48}'.format(String1),"##" print "#"*54 elif String3 == 'E': print "*"*54 print "**",'{0:^48}'.format(String2),"**" print "**",'{0:^48}'.format(String1),"**" print "*"*54 

Answer 1

Ok, the first thing is that the names are not at all descriptive. disfun? What does that mean? Maybe something like generateReport? What are the purposes of String1 and String2? Why does String2 get printed before String1? Why does String1 not get printed at all when String3 is "C"? Also, python variables tend to begin with a lowercase letter, not an uppercase letter.

As to shortening the function, there's a rule of thumb called DRY: Don't Repeat Yourself. If you see the same code written several times, extract it into a function.

Here's a first shot at improving your code:

def __printBannerEdge(char, width): assert len(char) == 1 print char * width def __printBannerContent(char, width, content): assert len(char) == 1 print char * 2, ('{0:^' + str(width - 6) + '}').format(content), char * 2 def __printBanner(char, width, lines): __printBannerEdge(char, width) for line in lines: __printBannerContent(char, width, line) __printBannerEdge(char, width) def generateReport(content, title, selection): if selection == "A" or selection == "B": if selection == "B": __printBannerEdge('#', 54) __printBannerContent('#', 54, '') __printBannerContent('#', 54, title) __printBannerContent('#', 54, content) __printBannerContent('#', 54, '') __printBannerEdge('#', 54) elif selection == "C": __printBanner('-', 40, [title]) elif selection == 'D': __printBanner('#', 54, [' * '.join(title), content]) elif selection == 'E': __printBanner('#', 54, [title, content]) 

Some commentary:

• The names I use (content, title, selection) are guess from context. There may be better names possible.
• I'm a little confused as to why the output is so different for each report type. I'm guessing that selection is set by user input from a menu of some sort? You should separate the function that parses user input from this function, and have 5 separate functions (eg generatePartList).
• One of the oddest thing about the function as written is that sometimes title is a single string, and sometimes it seems to be a list of strings (or maybe the join statement is meant to "bold" the title name?).
• Better than printing in these functions is to return a string, and let the caller print the string (that makes it easier to write your output to a file instead of standard output, for instance, if you decide later that's something you want to do).