Take the 2-minute tour ×
Code Review Stack Exchange is a question and answer site for peer programmer code reviews. It's 100% free, no registration required.

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
share|improve this question
    
I am at a loss why you'd call "String3" something that's clearly the type of banner, and "String2" to something that on one of the branches is joined (so it's in fact a collection). Very cryptic. –  tokland Aug 1 '13 at 12:26

1 Answer 1

up vote 2 down vote accepted

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).
share|improve this answer

Your Answer

 
discard

By posting your answer, you agree to the privacy policy and terms of service.

Not the answer you're looking for? Browse other questions tagged or ask your own question.