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

Chat / Discussions
,
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