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).
join
ed (so it's in fact a collection). Very cryptic. – tokland Aug 1 '13 at 12:26