2

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.

marked as duplicate by Robert Harvey, Doc Brown, gnat, Frank Hileman, Kilian Foth Nov 10 '17 at 9:48

This question has been asked before and already has an answer. If those answers do not fully address your question, please ask a new question.

  • 1
    There isn't enough context here to compose a reasonable response to your question; it depends on your specific circumstances. The short answer to "Is creating really small utility functions a bad idea" is "not if it effectively meets your requirements." – Robert Harvey Nov 9 '17 at 18:12
  • 1
  • 1
    or of this one: What is the ideal length of a method for you? – Doc Brown Nov 9 '17 at 18:23
  • 6
    I have learned in the course of people frequently asking this question that while most devs think your way is more readable, there seems to be one guy in every office who will always read right down to the bottom of the call stack, no matter how good your abstraction design or names are. For that one guy, more abstraction is worse and you will never be able to convince him otherwise. – Karl Bielefeldt Nov 9 '17 at 18:33
  • 2
    @KarlBielefeldt wow, I've never thought of it that way, but you are absolutely right. I wonder if it has to do with people have trust issues, or maybe not understanding abstraction as a concept. People are fascinating. – MetaFight Nov 9 '17 at 19:22
7

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.

10

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.

  • 1
    +1. I'd personally call it a domain-specific API though. – Panzercrisis Nov 9 '17 at 19:48
1

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.

  • There is no contradiction, but perhaps I have not been clear. We are currently only saving one thing, but I'd like to prepare for a situation where we would have more than one. Let me edit my question. – MatthewRock Nov 9 '17 at 18:14
1

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.

  • We are collaborating, so I'm trying to get the best approach. Choosing what I think is best doesn't always works, especially if I'm wrong. – MatthewRock Nov 10 '17 at 22:27
  • The best approach will be whatever the team agrees on. But this is a question of style, not correctness. There is no wrong answer here. – Solomon Slow Nov 11 '17 at 17:44

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