3

Let's say I want to have a function to load a project's data from both a json and a file path, the first idea which comes to my mind would be having a couple of functions such as:

def load_project_from_json(json_data):
    ...

def load_project_from_path(path):
    with open(path) as f:
        load_project_from_json(f.read())

but then I was asking myself if maybe would be more convenient to achieve both tasks with just a single function, for instance:

def load_project(path_or_json)

Many interrogants come to my mind, for instance:

  • how do you know when it's convenient to explode a function into multiple ones or not?

  • how can i judge whether a function is a "good" function or not?

  • is it the function following SRP?

Question: Which systematic method (set of steps or rules) could help me out to extract all responsabilities from any function? The main reason is learning how to judge the quality of any function (no matter if we're talking about free functions or class methods). That way I'll be able to say whether it's necessary or not to refactor it.

NS: When I say systematic method think in terms of simplifying equations, if you follow a set of rules you'll be able to reach a point where the equation can't be simplified further, similarly I'd like to know which rules I could apply to know when a function can't be refactored any further. I hope the analogy makes sense

  • 2
    Possible duplicate of Should I extract specific functionality into a function and why? – gnat Mar 13 '17 at 16:19
  • I'd suggest caution combining two different behaviors in a single function like that. Are you certain the two possible sets of arguments are entirely distinct? I don't see how you can rule out the possibility of a path that is also a valid json document. And what about error handling? What happens if you pass it something that is expected to be json content but turns out not to be, will it try opening it as a filename? – Jules Mar 13 '17 at 17:33
  • @Jules Try not focusing on that particular example too much, it was a simple analogy to give a little bit of context in the question. The main goal of the question is to figure out how to identify the responsabilites from a function. Said otherwise, how do you categorize responsabilities, which type of responsabilities exist, which is a responsability in the context of a free function, and so on... – BPL Mar 13 '17 at 17:37
  • @Jules I'm just trying to understand SRP applied to functions. For instance, what's the meaning of "there is more than one reason for it to change" in the sense of functions. – BPL Mar 13 '17 at 17:41
2

Having one function load from a file, and another load from JSON makes absolute sense. If your application finds itself in a situation where this is ambiguous -- and in multiple places -- then the third function can be added to remove the replication of logic determining which format it is: File or JSON.

Basically, you'll have all three where load_project(file_or_json) detects whether or not it's a file and calls the appropriate method:

def load_project(path_or_json)
  if os.path.exists(path_or_json):
     return load_project_from_path(path_or_json)
  else:
     return load_project_from_json(path_or_json)

Benefits:

  1. Centralized location of testing whether or not something is a file path or JSON

  2. It's handy to use when the caller doesn't know the difference

  3. You still have explicit functions when you do know the format

  4. You can optimize the test. Instead of calling out to the file system, maybe you decide to do a quick Regex test to see if it is JSON (after all, how many file names will start with { or [ characters?)

  5. You still adhere to the Single Responsibility Principal, because the load_project function is responsible for determining the format. The other two functions are responsible for loading from the specific formats.

  • Thanks for the answer. Btw, wouldn't that function violate the OCP? Each time you needed to add a new format you'd need to modify the function. In any case, it doesn't matter. The main purpose of my question is not about fixing that trivial function, that was just an example of mine to give a little bit of context. The main goal of the question is about figuring out responsabilites of any function (no matter which one). Because if you know how to identify responsabilities from functions then refactoring becomes a much easier task. So, what is it a responsability in the context of a function? – BPL Mar 14 '17 at 13:01
1

In general once you have removed any side effects from functions, ideally there shouldn't have been any in the first place, if the functions are performing identical tasks then, in dynamic or duck typing languages, there is no reason not to refactor into a single function.

Likewise, if a large portion of what two or more functions are doing, there can be a very good case for removing that/those portion(s) of the code to common functions, e.g. loading different types of files in specific functions but manipulating the contents in common functions.

In languages without dynamic typing the type safety often means that you need duplicate code, with different types and it is a bad idea to weaken the type safety to save some code. Some languages have the variant mechanisms which allow functions to operate on a specific sub-set of types which can be used or, of course, refactoring into classes where the base class provides the common functionality is often a good idea but it can be carried to excess - I have seen some code with 100s of classes each with about 10 lines of code in which is nearly as bad as having a single function of 3000+ lines duplicated in 3 files, (both examples were by the same authors).

Of course in C++, and some other languages, there is the idea of templates which allows you to, potentially, create 100s of classes/functions - each technically type safe - automatically.

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.