When should you make a new function vs adding logic to an existing function?

Chat / Discussions
,
1

I’m currently working on a codebase written by somebody else and I have noticed that they include some very long single functions. Part of my task is to refactor these and clear up unsafe variable deprecations which is really helping my Elixir learning but it has raised an issue I’d like some guidance on.

Are there any conventions around when you should create a seperate function for a piece of logic vs just adding more logic to an existing function?

I have an example like this:

def function do
    # a lot of things
    
    if var1 == 1 or var2 == 1 or var3 == 1 or var4 == 1 or var 5 == 1 do
        another_variable = true
    end

    # a lot of things

end

My intention is to create a list of the vars and pass it to a private function like this:

defp set_another_variable(list) do
    Enum.any?(list, fn x -> == 1 end)
end

With result being assigned to the another variable

This has made me think…do I need another function here? Should I just add the Enum logic to the existing function? It’s such a small piece of code am I contributing to a ‘function soup’ if I extrapolate this strategy across a whole module?

My inclination is that it is better to have a series of smaller functions that you can ctlr + f to and see their logic rather than having long functions (as I have struggled with this in this very project) but I’d love to get some insight from more experienced devs.

2

I think it was Martin Fowler(or maybe Dave Thomas?) who said something along the lines of write it once, copy and paste it a second time, make it a function the third time and I generally agree with that for most situations.

The only time I differ is if its a complex or confusing bit of code I like to separate it out so in the main flow I just have to understand the function name not try to reason about the code.

That said if the functionality is outside the scope implied by the function name I move it out as well.

1 Like
3

By anonymizing the code you have hidden the primary benefit of Extract Function:

def function fun do
  # a lot of things
    
  if var1 == 1 or var2 == 1 or var3 == 1 or var4 == 1 or var5 == 1 do
    another_variable = true
  end

  # a lot of things

end

def function fun do
  # a lot of things
    
  another_variable = set_another_variable(var1, var2, var3, var4, var5, another_variable)

  # a lot of things

end

defp set_another_variable(var1, var2, var3, var4, var5, default \\ false),
  do: var1 == 1 or var2 == 1 or var3 == 1 or var4 == 1 or var5 == 1 or default

Refactoring: Improving the Design of Existing Code, 2e p.107:

The argument that makes most sense to me, however, is the separation between intention and implementation. If you have to spend effort looking at a fragment of code and figuring out what it’s doing, then you should extract it into a function and name the function after the “what.” Then, when you read it again, the purpose of the function leaps right out at you, and most of the time you won’t need to care about how the function fulfills its purpose (which is the body of the function).

set_another_variable is about as useful as a comment # setting another_variable - it gives no insight as to how those 5 variables relate to another_variable in terms of the problem being solved.

The core requirement of Extract Function is to give the function a meaningful name that explains the “what” and the “why” in terms of the problem being solved, so that the “how” (the actual code/implementation) is not relevant to understanding.

Also in this particular context Enum.any?(list, fn x -> x == 1 end) seems “over-engineered” unless there is another good reason to collect those values in a list.

Furthermore

    if var1 == 1 or var2 == 1 or var3 == 1 or var4 == 1 or var 5 == 1 do
        another_variable = true
    end

is

    if var1 == 1 or var2 == 1 or var3 == 1 or var4 == 1 or var 5 == 1 do
        another_variable = true
    else
        nil
    end

An if expression isn’t an if statement - and it will produce a warning with newer versions of Elixir.

The likely intention was

  another_variable =
    if var1 == 1 or var2 == 1 or var3 == 1 or var4 == 1 or var 5 == 1 do
        true
    else
        another_variable
    end

which is (assuming another_variable is boolean)

  another_variable = var1 == 1 or var2 == 1 or var3 == 1 or var4 == 1 or var 5 == 1 or another_variable

I personally don’t think that

  another_variable = Enum.any?([var1, var2, var3, var4, var5, another_variable], fn x -> x == 1 end)

is any better in the “intention revealing” department and once the function is extracted

defp set_another_variable(var1, var2, var3, var4, var5, default \\ false),
  do: var1 == 1 or var2 == 1 or var3 == 1 or var4 == 1 or var5 == 1 or default

is simple enough already (even if it looks pedestrian).

Paraphrasing:

Extracting the function creates the opportunity to maximize the ignorance of how the function works and to emphasize why it exists.

1 Like
4

Thanks for putting so much effort into your response. I understand your point about anonymity the code and that is definitely something I will keep in mind next time I’m posting here or somewhere like SO.

I wasn’t actually looking for an answer to the specific problem posed and was intending to just use the pseudo code as an example of ‘Should I create a new function here?’ generally -I was obviously too casual with it!

Having said that, your response to the specific problem is a nice bonus and I agree with you that actually adding a step (creating a list) and invoking Enum isn’t providing much and is maybe more obscuring than staying explicit as per your example.

Really eye opening all around.