1
\$\begingroup\$

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
\$\endgroup\$
1
  • \$\begingroup\$ 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. \$\endgroup\$
    – tokland
    Commented Aug 1, 2013 at 12:26

1 Answer 1

2
\$\begingroup\$

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).
\$\endgroup\$

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge you have read our privacy policy.

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