Is creating really small utility functions a bad idea? [duplicate]

Question

Recently, I've been talking with a friend about the code I've written. The code looked something like:

import json

def save_data(destination, data):
  with open(destination, 'w') as out:
    json.dump(data,out, ensure_ascii=False)

# Code...

save_data(path,data)

Similar for loading the file.

My friend argued that it's not much of a save(I agree, that wasn't my point). He also said that it makes a code harder to read, because you have to look up the definition of save_data to know what it does(I disagree) and suggested just leaving the code instead of moving it to a function.

I think that the code is at least as readable, potentially reduces redundancy(perhaps we will need to save more than one file) and could potentially help during refactorization(instead of modifying every place where the file is saved, we'd just change save_data instead), even though right now we only save one thing to a file.

Is it really a bad idea to move even small, once-used pieces of code to functions?

I have tagged the question with Python, since the code is Python, but I think that the question is universal.

Answer 1

Size is irrelevant. You should be thinking in terms of levels of abstraction and points of potential change. At that point, there is no minimum size of a function. The function should capture an idea. For example,

def normalize(s):
    return s.strip()

In this case, normalize is essentially just an alias for strip. However, strip operates at the level of string manipulation, while normalize conveys the notion that there are non-normalized and normalized representations. Conceptually, normalize is just an operation on an abstract data type that just happens to be being represented by a string. It's also quite possible that what is necessary to "normalize" a string in the relevant sense might change. How often a function is used in this context is irrelevant. If I make a Stack abstract data type, I don't inline the definition of a pop operation, just because it's used only once.

On the other hand, oftentimes functions functions capture commonly reoccurring patterns for convenience. These are functions that are defined in terms of functions at one level of abstraction and produce a function at the same level of abstraction. You should still endeavor to have these functions capture a specific idea, and that may suggest replacing the reoccurring pattern with a smaller/simpler pattern of functions rather than just a single function. That said, a commonly occurring pattern usually indicates that there is some idea that can be captured. For these types of functions, how often they occur does matter. It doesn't make much sense to make a convenience function for something that rarely occurs. It also isn't a good trade in time and cognitive load to make someone look up and remember the definition of an operation that is rarely used when you could just inline the definition.

Your question gives too little context to decisively provide an answer. If save_data is part of a barrier of abstraction from how the data is stored, then you absolutely should have it regardless of how short it is. However, in that case, there should (probably) be corresponding operations that are also part of that abstraction barrier that read the data in. It should be possible to, for example, switch from JSON to XML just by modifying the functions of the abstraction barrier and none of the consuming functions. If this is not the case, then you either need to raise the level of abstraction elsewhere, or save_data isn't serving an abstraction purpose, and may well be hiding relevant details and thus shouldn't exist.

If you don't expect save_data to be used elsewhere and it isn't serving as part of an abstraction barrier, then it probably shouldn't exist. By pulling code out into a function, you are stating that the details aren't that important to the consumer. However, if those details are important, they shouldn't be hidden. For example, if save_data is used inside a serialize_to_json method, hiding the fact that save_data does actually write out JSON doesn't make sense. And if you decide, serialize_to_json should be serialize_to_xml, you shouldn't need to go to some other function to change that detail. So, in this case, I would agree with your colleague that the function's definition should be inlined and the function shouldn't exist.

Answer 2

In my experience factoring out small functions reduces the total code size and increases velocity. It's true that to know what is happening in save_data then one has to go read the save_data function. But often when reading a high-level function we only care about one particular step. It's much easier to comprehend:

a = get_a(...)
b = get_b(...)
c = get_c(...)
save_data(a, b, c)

Than:

// get_a (this comment is present if you are lucky)
... ten lines of code, introducing additional local vars x and y
// get_b
... fifteen lines, introducing p and q
// get_c
...
// save_data
... twenty lines involving f,g,h,i,j

Reading the inlined code requires that the reader verify that the comments are correct (if they are present at all).

In real systems these small functions end up being reused more than initially expected. Factoring out small functions gradually creates a domain-specific language, moving developer focus from low-level implementation details to higher level domain-specific objects.

Answer 3

You've already conflicted yourself, you say:

Is it really a bad idea to move even small, once-used pieces of code to functions?

but earlier said:

instead of modifying every place where the file is saved

So, which is it, is it used once, or multiple times?

I would advocate the YAGNI approach. If your json saving really only exists in one place, and it is a simple, no-decision making process, and you don't know for certain you'll do it elsewhere, leave it where it is. The moment something else needs to save the json, then extract out the method and begin abstracting the save process.

Answer 4

He ... said that it makes a code harder to read

OK, and what do you say? Look at the call sites. If you think that having a save_data(...) function improves the readability of functions that call it, then it's a win for you.

Is it really a bad idea to move even small, once-used pieces of code to functions?

I think you'll have to answer that on a case-by-case basis. I've often refactored big complicated functions into sequences of calls to smaller functions, and considered it to be an improvement. But I've also seen cases where moving a few lines of code that aren't all that closely related to each other into a subroutine with an ill-considered name only seemed to hide what was going on.