Is it always a best practice to write a function for anything that needs to repeat twice?

Question

Myself, I can't wait to write a function when I need to do something more than twice. But when it comes to things that only appear twice, it's a bit more tricky.

For code that needs more than two lines, I'll write a function. But when facing things like:

print "Hi, Tom"
print "Hi, Mary"

I'm hesitant to write:

def greeting(name):
    print "Hi, " + name

greeting('Tom')
greeting('Mary')

The second one seems too much, doesn't it?

---

But what if we have:

for name in vip_list:
    print name
for name in guest_list:
    print name

And here is the alternative:

def print_name(name_list):
    for name in name_list:
        print name

print_name(vip_list)
print_name(guest_list)

Things become tricky, no? It's hard to decide now.

What's your opinion about this?

Answer 1

Although it's one factor in deciding to split off a function, the number of times something is repeated shouldn't be the only factor. It often makes sense to create a function for something that's only executed once. Generally, you want to split a function when:

• It simplifies each individual abstraction layer.
• You have good, meaningful names for the split-off functions, so you don't usually need to jump around between abstraction layers to understand what's going on.

Your examples don't meet that criteria. You're going from a one-liner to a one-liner, and the names don't really buy you anything in terms of clarity. That being said, functions that simple are rare outside of tutorials and school assignments. Most programmers tend to err too far the other way.

Answer 2

Only if the duplication is intentional rather than accidental.

Or, put another way:

Only if you would expect them to co-evolve in the future.

---

Here's why:

Sometimes, two pieces of code just happen to become the same even though they have nothing to do with each other. In that case you must resist the urge to combine them, because the next time someone performs maintenance on one of them, that person will not expect the changes to propagate to a previously-nonexistent caller, and that function may break as a result. Hence you have to only factor out code when it makes sense, not whenever it seems to reduce code size.

Rule of thumb:

If the code only returns new data and doesn't modify existing data or have other side effects, then it's very likely to be safe to factor out as a separate function. (I can't imagine any scenario in which that would cause a breakage without entirely changing the intended semantics of the function, at which point the function name or signature should change too, and you'd need to be careful in that case anyway.)

Answer 3

No, it's not always a best practice.

All other things being equal, linear, line-by-line code is easier to read than jumping around function calls. A non-trivial function call always takes parameters, so you have to sort all that out and make mental contextual jumps from function call to function call. Always favor better code clarity, unless you have a good reason for being obscure (such as obtaining necessary performance improvements).

So why is code refactored into separate methods then? To improve modularity. To collect significant functionality behind an individual method and give it a meaningful name. If you're not accomplishing that, then you don't need those separate methods.

Answer 4

In your particular example making a function may seem overkill; instead I would ask the question: is it possible that this particular greeting will change in the future? How so?

Functions are not simply used to wrap up functionality and ease reuse, but also to ease modification. If requirements change, copy-pasted code will need to be hunted down and changed manually, while with a function the code needs to be modified only once.

Your example would benefit from this not via a function - since the logic is nearly non-existent - but possibly a GREET_OPENING constant. This constant could then be loaded from file so that your program would be easily adaptable to different languages, for example. Note that this is a crude solution, a more complex program would probably need a more refined way to track i18n, but again it depends on requirements vs work to do.

It's all about possible requirements in the end, and planning things in advance to ease your future self's job.

Answer 5

Personally I have adopted the rule of three - which I'll justify by calling YAGNI. If I need to do something once I'll write that code, twice I may just copy / paste (yes I just admitted to copy/paste!) because I ain't gonna need it again, but if I need to do the same thing again then I'll refactor and extract that chunk into it's own method, I've demonstrated, to myself, that I will need it again.

I agree with what Karl Bielefeldt and Robert Harvey have said and my interpretation of what they're saying is that the overriding rule is readability. If it makes my code easier to read then consider creating a function, keep in mind things like DRY and SLAP. If I can avoid having changing level of abstraction in my function then I find that easier to manage in my head, so not jumping between functions (if I can't understand what the function does simply by reading it's name) means fewer switches in my mental processes.
Likewise not having to switch context between functions and inline code such as print "Hi, Tom" works for me, in this case I might extract a function PrintNames() iff the rest of my overall function were mostly function calls.

Answer 6

It's seldom clear-cut, so you need to weigh options:

• Deadline (fix burning server room ASAP)
• Code readability (may influence choice either way)
• Shared logic's level of abstraction (related to above)
• Re-use requirement (i.e. is having exact same logic important, or just convenient right now)
• Difficulty of sharing (here is wher python shines, see below)

In C++, I usually go by the rule of three (i.e. the third time I need the same thing, I refactor it into a properly sharable piece), but with experience it's easier to make that choice initially as you know more about the scope, software & domain you're working with.

However, in Python it is fairly light-weight to reuse, rather than repeat, logic. More so than in many other languages (at least historically), IMO.

So, consider just re-using the logic locally, for instance by by creating a list from local arguments:

def foo():
    for name_list in (vip_list, guest_list): # can be list of tuples, for many args
        for name in name_list:
            print name

You can use a tuple of tuples, and split it directly into the for-loop, if you need several arguments:

def foo2():
    for header, name_list in (('vips': vip_list), ('people': guest_list)): 
        print header + ": "
        for name in name_list:
            print name

or make a local function (maybe your second example is that), which makes the logic re-use explicit but also clearly shows that print_name() isn't used outside the function:

def foo():
    def print_name(name_list):
        for name in name_list:
            print name

    print_name(vip_list)
    print_name(guest_list)

Functions are preferable especially when you need to abort the logic in the middle (i.e. use return), as break or exceptions might clutter things unnecessarily.

Either is superior to repeating the same logic, IMO, and also less cluttering than declaring a global/class function that is only used by one caller (albeit twice).

Answer 7

All best-practices have a particular reason, which you can refer to in order to answer such a question. You should always avoid ripples, when one potential future change implies changes to other components as well.

So in your example: If you assume that you have a standard greeting and you want to ensure it's the same for all people, then write

def std_greeting(name):
    print "Hi, " + name

for name in ["Tom", "Mary"]:
    std_greeting(name)   # even the function call should be written only once

Otherwise, you'd have to pay attention and change the standard greeting at two places, if it happens to change.

If, however, the "Hi" is the same just by accident and changing one of the greetings does not necessarily result in changes for the other, then keep them separate. Therefore, keep them separate if there are logical reasons to believe, that the following change is more likely:

print "Hi, Tom"
print "Hello, Mary"

For your second part, to decide how much to "package" into functions probably depends on two factors:

• keep blocks small for readability
• keep blocks large enough so that changes occur in only few blocks. Not need to cluster too much as it's hard to track the calling pattern.

Ideally, when a change occurs you will think "I have to change most of the following block" and not "I have to change code somewhere inside a large block".

Answer 8

The most important aspect of named constants and functions isn't so much that they reduce the amount of typing, but rather that they "attach" the different places where they are used. With regard to your example, consider four scenarios:

1. It's necessary to change both greetings in the same way, [e.g. to Bonjour, Tom and Bonjour, Mary].

2. It's necessary to change one greeting but leave the other as it is [e.g. Hi, Tom and Guten Tag, Mary].

3. It's necessary to change both greetings differently [e.g. Hello, Tom and Howdy, Mary].

4. Neither greeting ever needs to be changed.

If it's never necessary to change either greeting, it won't really matter which approach is taken. Using a shared function will have the natural effect that changing any greeting will change them all the same way. If all greetings should change the same way, that would be a good thing. If they shouldn't, however, then each person's greeting will have to be coded or specified separately; any work done making them use a common function or specification will have to be undone (and would have been better not done in the first place).

To be sure, it's not always possible to predict the future, but if one has reason to believe that it's more likely that both greetings will need to change together, that would be a factor in favor of using common code (or a named constant); if one has reason to believe that it's more likely that one or both might need to change so that they differ, that would be a factor against trying to use common code.

Answer 9

I think you should have a reason to create a procedure. There are a number of reasons to create a procedure. The ones that I think are most important are:

1. abstraction
2. modularity
3. code navigation
4. update consistency
5. recursion
6. testing

Abstraction

A procedure can be used to abstract a general algorithm from the details of particular steps in the algorithm. If I can find a suitably coherent abstraction, this helps me to structure the way I think about what the algorithm does. For example, I can design an algorithm that works on lists without having to necessarily consider the list representation.

Modularity

Procedures can be used to organise code into modules. The modules can often be treated separately. For example, built and tested separately.

Well designed module typically capture some coherent meaningful unit of functionality. Therfore they can often be owned by different teams, replaced completely with an alternative or reused in a different context.

Code Navigation

In large systems, finding the code associated with a particular system behaviour can be challenging. Hierarchical organisation of the code within libraries, modules and procedures can help with this challenge. Particularly if you try to a) organise functionality under meaningful and predictable names b) place the procedures relating to similar functionality near each other.

Update Consistency

In large systems it can be challenging to find all the code that needs to be changed to achieve a particular change in behavior. Using procedures to organise the functionality of the program can make this easier. In particular, if each bit of functionality of your program appears only once and in a cohesive group of procedures it is less likely (in my experience) that you will miss somewhere you should have updated or make an inconsistent update.

Note that you should organise procedures based on the functionaility and abstractions in the program. Not based on whether two bits of code happen to be the same at the moment.

Recursion

Using recursion requires that you create procedures

Testing

You can usually test procedures independently of each other. Testing different parts of the body of a procedure independently is more difficult, because you typically have to execute the first part of the procedure before the second part. This observation also often applies to other approaches to specifying/verifying programs behavior.

Conclusion

Many of these points relate to the comprehensibility of the program. You could say that procedures are a way to create a new language, specific to your domain, with which to organise, write, and read, about the problems and processes in your domain.

Answer 10

Neither of the cases you give seem worth refactoring.

In both cases, you are performing an operation which is expressed clearly and succinctly, and there is no danger of an inconsistent change being made.

I recommend you always look for ways to isolate code where:

1. There is an identifiable, meaningful task which you could separate out, and

2. Either

   a. the task is complex to express (taking into account the functions available to you) or

   b. the task is performed more than once, and you need a way of making sure it is changed the same way in both places,

If condition 1 is not satisfied then you will not find the right interface for the code: if you try to force it you will end up with loads of parameters, multiple things you want to return, lots of contextual logic, and will quite possibly be unable to find a good name. Try documenting the interface you are thinking of first, it's a good way of testing the hypothesis that it's the right bundling of functionality and comes with the added bonus that when you write your code, it's already specced and documented!

2a is possible without 2b: sometimes I have taken code out the flow even when I know it only gets used once, simply because moving it elsewhere and replacing it with a single line means that the context in which it is called is suddenly much more readable (especially if it's an easy concept which the language happens to make difficult to implement). It's also clear when reading the extracted function where the logic begins and ends and what it does.

2b is possible without 2a: The trick is having a feel for what sort of changes are more likely. Is it more likely that your company policy will change from saying "Hi" everywhere to "Hello" all the time or that your greeting changes to a more formal tone if you're sending a demand for payment or an apology for a service outage? Sometimes it might be because you're using an external library you're not sure about and want to be able to swap that for another one quickly: the implementation might change even if the functionality doesn't.

Most often, though, you'll have some a mixture of 2a and 2b. And you'll have to use your judgment, taking into account the business value of the code, the frequency it gets used, whether it's well-documented and understood, the state of your test suite, etc.

If you notice the same bit of logic used more than once, by all means you should take the opportunity to consider refactoring. If you're beginning new statements more than n indentation levels in in most languages, that's another, slightly less significant trigger (pick a value of n you're comfortable with for your given language: Python might be 6, for instance).

So long as you're giving thought to these things and taking down the big spaghetti-code monsters, you should be fine - don't spend so long worrying about how fine-grained your refactoring needs to be that you don't have time for things like tests or documentation worked up about. If it does need doing, time will tell.

Answer 11

In general I agree with Robert Harvey, but wanted to add a case to split up a functionality into functions. To improve readability. Consider a case:

def doIt(smth,smthElse)
    for x in getDataFromSomething(smth,smthElse):
        if not check(x,smth):
            continue
        process(x,smthElse)
        store(x) 

Even though those function calls are not used anywhere else, there is significant gain in readability if all 3 functions are considerably long and have nested loops etc.

Answer 12

There is no hard and fast rule that must be applied, it will depend on the actual function that would be used. For simple name printing I would not use a function but if it is a maths sum that would be a different story. You then would create a function even if it only gets called twice, this is to ensure that the maths sum is always the same if ever changed. In another example if you are doing any form of validation you would use a function, so in your example if you needed to check the name is longer than 5 characters you would use a function to ensure the same validations are always done.

So I think you answered your own question when you stated "For codes that need more than two lines, I'll got to write a func". In general you would be using a function but you must also use your own logic to determine if there is any kind of value add from using a function.

Answer 13

I have come to believe something about refactoring that I haven't seen mentioned here, I know there are a lot of answers here already, but I think this is new.

I've been a ruthless refactorer and a strong believer in DRY since before the terms arose. Mostly it's because I have trouble keeping a large codebase in my head and partly because I enjoy DRY coding and I do not enjoy anything about C&P coding, in fact it's painful and terribly slow for me.

The thing is, insisting on DRY has given me a lot of practice in some techniques that I rarely see others use. A lot of people insist that Java is difficult or impossible to make DRY, but really they just don't try.

One example from a long time ago that's somewhat similar to your example. People tend to think java GUI creation is hard. Of course it is if you code like this:

Menu m=new Menu("File");
MenuItem save=new MenuItem("Save")
save.addAction(saveAction); // I forget the syntax, but you get the idea
m.add(save);
MenuItem load=new MenuItem("Load")
load.addAction(loadAction)

Anyone who thinks this is just crazy is absolutely right, but it's not Java's fault--code should never be written this way. These method calls are functions intended to be wrapped in other systems. If you can't find such a system, build it!

You obviously can't code like that so you need to step back and look at the problem, what is that repeated code actually doing? It's specifying some strings and their relationship(a tree) and joining the leaves of that tree to actions. So what you really want is to say:

class Menu {
    @MenuItem("File|Load")
    public void fileLoad(){...}
    @MenuItem("File|Save")
    public void fileSave(){...}
    @MenuItem("Edit|Copy")
    public void editCopy(){...}...

Once you've defined your relationship in some way that is succinct and descriptive then you write a method to deal with it--in this case you iterate over the methods of the class passed in and build a tree then use that to build your Menus and Actions as well as (obviously) display a menu. You'll have no duplication, and your method is reusable... and probably easier to write than a large number of menus would have been, and if you actually enjoy programming, you had a LOT more fun. This isn't hard--the method you need to write is probably less lines than creating your menu by hand would be!

Thing is, in order to do this well you need to practice, a lot. You need to get good at analyzing exactly what unique information is in the repeated parts, extract that information and figure out how to express it well. Learning to use tools like string parsing and annotations helps a lot. Learning to be really clear about error reporting and documentation is very important as well.

You get "Free" practice simply by coding well--heck chances are that once you get good at it you'll find that coding something DRY (including writing a reusable tool) is faster than copying and pasting and all the errors, duplicated errors and difficult changes that type of coding causes.

I don't think I'd be able to enjoy my job if I didn't practice DRY techniques and building tools as much as I could. If I had to take a cut in pay not to have to do copy and paste programming, I'd take it.

So my points are:

• Copy & Paste costs more time unless you don't know how to refactor well.
• You learn to refactor well by doing it--by insisting on DRY even in the most difficult and trivial of cases.
• You are a programmer, if you need a small tool to make your code DRY, build it.

Answer 14

Generally, it's a good idea to split something into a function of it improves the readability of your code, Or, if the part being oft repeated is somehow core to the system. In the above example, If you needed to greet your users depending on the locale, then it would make sense to have a separate greeter function.

Answer 15

As a corollary to @DocBrown's comment to @RobertHarvey about using functions to create abstractions: if you can't come up with an adequately informative function name that isn't significantly more concise or clearer than the code behind it, you are not abstracting much. Absent any other other good reason for making it a function, there is no need to do so.

Also, a function name rarely captures its full semantics, so you may have to go check its definition, if it isn't commonly-enough used to be familiar. Particularly in other than a functional language, you may need to know if it has side-effects, what errors may occur and how they are responded to, its time complexity, whether it allocates and/or frees resources, and whether it is thread-safe.

Of course, we are by definition considering only simple functions here, but that goes both ways - in such cases, inlining is not likely to add complexity. Furthermore, the reader probably will not realize that it is a simple function until he looks. Even with an IDE that hyperlinks you to the definition, the visual jumping around is an impediment to understanding.

Generally speaking, recursively factoring code into more, smaller functions will eventually bring you to the point where the functions no longer have independent meaning, because as the code fragments they are made from get smaller, those fragments get more of their meaning from the context created by the surrounding code. As an analogy, think of it like a picture: if you zoom in too closely, you can't make out what you are looking at.