2

A method grew too big for its own good, and I need to break it up into two separate methods.

def big_method(dct):

    # Initial code
    # ...
    for i in dct:
        # More code
        # ...
        for j in dct[i]:    
            # Inner code
            # ...
            # ...
            # and a break
            if j == 'something':
                break
            # Inner code ended here.

The problem is that my first method will contain a loop, and the second method needs to contain a break:

def first_small(dct, second_method):
    # Initial code
    # ...
    for i in dct:
        # More code
        # ...
        for j in dct[i]:
            second_method()


def second_small(j):

    # Plenty of code
    # ...
    # ...
    # and a break
    if j == 'something':
        break    
    # Inner code ended here.

second_small() would raise a SyntaxError: 'break' outside loop.

To fix it, I can replace break in second_small() with a custom exception and make first_small() handle that exception, and break its loop when it finds it:

def first_small(dct, second_method):
    # Initial code
    # ...
    for i in dct:
        # More code
        # ...
        for j in dct[i]:
            try:
                second_method(j=j)
            except MyException:
                break


def second_small(j):

    # Inner code
    # ...
    # ...
    # and a break
    if j == 'something':
        raise MyException
    # Inner code ended here.

However, I am not so sure this is the cleanest way to achieve that.

Question:
How should I split a method into two others, where first method will contain a loop and second has to break that loop?

7

Forget the outer loop around for a moment, and think about the meaning of your method second_small, the abstraction it represents (I assume in your real code, you picked a more meaningful name). Then what you do in that method should depend on what

 if j == 'something':
     ...

means, in contrast to that abstraction. For example, if j == 'something' is a real error case, throw an exception. If it means "I successfully found the result of a search", use a boolean return value and return True. If it means "could not find it", return False. If it means "validation succeeded", return True.

A boolean return value might be also the better alternative if the condition means "might be an error, not sure, only the caller can decide". A boolean is probably the wrong alternative if you have to provide some kind of error message.

It should be clear in case of using a boolean, your code will finally look like this:

 for j in dct[i]:
      if second_method(j):
            break

def second_method(j):
    # ...
    if j == 'something':
        return True
    # ...
  • This solution indeed seems much better. Would you recommend a boolean as you said, or would second_method() returning a string like "break" make it more explicit? – Fermi paradox May 23 '15 at 10:14
  • 1
    @user5061: you missed my point, and it seems you ignored my first sentence. What I tried to tell you is that from an unspecific name like second_method or small_method one cannot deduce the "correct" kind of return value. You have to look at your specific methods, think if you broke your big method down into not just smaller, but meaningful methods, and then you can decide what's best to return. Maybe its an exception, maybe a boolean, maybe something different, but most probably not "break", since this would make assumptions about the calling context. – Doc Brown May 23 '15 at 11:09
  • The actual names I use are meaningful. Perhaps i should have chosen more suitable names for my question, like cook_all_dishes() and cook_single_dish(). My small hesitation with returning True and False comes from the fact cook_single_dish() feels like it should only be modifying dicts etc, not returning something. Perhaps i should change that name. – Fermi paradox May 23 '15 at 11:36
  • @user5061: I think your question is fine as it is, since it allowed me to give a more general explanation for different cases. What you have to decide is (in your real world case), what the condition j==something means in context of a method like cook_single_dish. Maybe it means "dish completed", or it means "cooking was not possible because the stove is broken", or "cooking was interrupted because estimated time is over", or "number of ordered dishes is reached". Depending on that meaning, you should decide about the kind of return value. – Doc Brown May 23 '15 at 18:32

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

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