Avoiding duplicate code in methods

Sometimes in the body of a method implementing a somewhat complex algorithm, duplication of lines of code cannot be easily avoided.

The availability of nested methods would seem to be an adequate response to this problem.

Is there or has there been any thought given to this for the Crystal language?

Perhaps not the answer you were looking for, but will closures fit the need?

def my_func(x)
  aux_f = ->(x : Int32) {x + 1 }
  aux_f.call (aux_f.call x)
end

p my_func 10  # => 12

Yes, they do, and that’s what I’ve used so far.
The downside (for me) is that they don’t share the environment of the parent method and require passing parameters.
What I’m talking about, and which seems much more flexible to me, is what is described in the Wikipedia article.
But, using Proc is already a good option :slight_smile:

I suppose to some extend you could use macros for that. They expand into the method scope, so they have access to all local variables and don’t create closures.

macro aux_f(x)
  {{ x }} + y
end

def my_func(x)
  y = 1
  aux_f aux_f x
end

p my_func 10  # => 12

And that’s really the main purpose of macros: Reduce repetition in code.
The only thing that’s a bit odd with that is that the macro is also visible outside the method.

They do share the local context (that’s actually why they’re called “closures”: they enclose the local environment).

def my_func(z)
  aux_f = ->(x : Int32) {x + z}
  aux_f.call (aux_f.call 2)
end

p my_func 10 # => 22

Or am I getting you wrong?

1 Like

@straight-shoota : a macro works well, except that not only, as you said, it is visible outside the method, but it cannot be defined inside the method, which, in my opinion, render the code less readable. Also, I guess the compiled code is duplicated each time the macro is used.
@beta-ziliani Closures do share environment indeed, but only if defined before the closure, which is not always suitable for highly nested portions of code.
So, it seems there is not a clear winner :thinking:
Thank you anyway for your advice.

1 Like

Closures do share environment indeed, but only if defined before the closure

Indeed, but note this is also true for every language mentioned in the Wikipedia article.

The best way to describe what you need is local macros then.

1 Like

The closest thing would be private methods next to the main method.

I personally never understood the appeal of nested methods: they further indent the code and they make the main code look longer and convoluted.

2 Likes

IMO it gives structure. It might make the code longer, but I would say it’s more organized, not convoluted.

That’s not necessarily a bad thing, is it? It means the code can be more optimized for each specific use case.

Hmm, so the use of a macro seems to be the best compromise at the moment, but a “nested defs” implementation would be, in my opinion, an interesting addition to the language.

private def may require modifying existing code beforehand, for example when it contains statements such as i += 1 (Error: ‘+=’ before definition of ‘i’) and closures are not suitable for deeply nested code.

I don’t understand the difference between a private def and a closure/proc.

Could you put a short method definition here that shows how you would like it to look?

1 Like

This question is not clear, i consider it is better to paste the code here, then we can give some advice for use Proc or Macro.

a macro works well, except that not only, as you said, it is visible outside the method, but it cannot be defined inside the method,

Using a macro doesn’t mean you have to use the macro keyword, you can defined a BIG method use macro, with very less code. this is a example, maybe it is what you want.

@zw963 My original idea was to simply avoid repeating the same lines of code in several places in the body of the method (DRY !)
I hadn’t considered the approach you suggest: perhaps it’s a more elegant solution if it’s suitable in my case.
Thanks

@hotou Could you show a code snippet? Do you need multiple nested methods or just one? If it’s just one using blocks can greatly help in avoiding code duplication. But… it depends on the exact code.

Here is a piece of code that I have adapted to illustrate my initial question.

def calculate_cells(formatted_content, maxwidth, wrap_preserve)
  cells = [] of String
  line_index = 0
  case wrap_preserve
  when :char
    formatted_content.split(/\n/).flat_map do |substr|
      cell_line = String::Builder.new
      cell_line_width = 0
      substr.scan(/\X/).each do |r|
        char = r[0]
        char_width = char.size
        if cell_line_width + char_width > maxwidth
          cells << cell_line.to_s
          line_index += 1
          # <<< 3 lines to be duplicated
          cell_line.close
          cell_line_width = 0
          cell_line = String::Builder.new
          # >>>
        end
        cell_line << char
        cell_line_width += char_width
      end
      cells << cell_line.to_s
      cell_line.close
      line_index += 1
    end
  when :word
    formatted_content.split(/\n/).flat_map do |substr|
      cell_line = String::Builder.new
      cell_line_width = 0
      substr.split(/(?<= |\-|\—|\–⁠)\b/).each do |word|
        word_width = word.size
        combined_width = cell_line_width + word_width
        if combined_width - 1 == maxwidth && word[-1] == " "
          # do nothing
        elsif combined_width > maxwidth
          content = cell_line.to_s
          if content.strip.size != 0
            cells << content
            line_index += 1
          end
          # <<< 3 duplicated lines
          cell_line.close
          cell_line_width = 0
          cell_line = String::Builder.new
          # >>
        end
        if word_width >= maxwidth
          word.scan(/\X/).each do |r|
            char = r[0]
            char_width = char.size
            if cell_line_width + char_width > maxwidth
              if char != " "
                cells << cell_line.to_s
                line_index += 1
                # <<< 3 duplicated lines
                cell_line.close
                cell_line_width = 0
                cell_line = String::Builder.new
                # >>>
              end
            end
            cell_line << char
            cell_line_width += char_width
          end
        else
          cell_line << word
          cell_line_width += word_width
        end
      end
      cells << cell_line.to_s
      line_index += 1
      cell_line.close
    end
  end
  cells
end

formatted_line = "Crystal language is the best!\nThere is nothing to discuss!"

puts formatted_line
puts calculate_cells(formatted_line, 12, :char)
puts calculate_cells(formatted_line, 12, :word)

The purpose of the calculate_cells method is to transform a string into an array of strings of a given maximum length, by cutting the original string either at a character or at a word boundary if possible.

As you see, there are 3 identical lines of code in 3 different places.

With the use of a nested method sharing the data of the enclosing method, we could write:

def calculate_cells(formatted_content, maxwidth, wrap_preserve)
  
  def new_cell_line(check_size)
    content = cell_line.to_s
    if (check_size && content.strip.size != 0) || !check_size
      cells << content
      line_index += 1
    end
    cell_line.close
    cell_line_width = 0
    cell_line = String::Builder.new
  end

  cells = [] of String
  line_index = 0
  case wrap_preserve
  when :char
    formatted_content.split(/\n/).flat_map do |substr|
      cell_line = String::Builder.new
      cell_line_width = 0
      substr.scan(/\X/).each do |r|
        char = r[0]
        char_width = char.size
        if cell_line_width + char_width > maxwidth
          new_cell_line(false)
        end
        cell_line << char
        cell_line_width += char_width
      end
      cells << cell_line.to_s
      cell_line.close
      line_index += 1
    end
  when :word
    formatted_content.split(/\n/).flat_map do |substr|
      cell_line = String::Builder.new
      cell_line_width = 0
      substr.split(/(?<= |\-|\—|\–⁠)\b/).each do |word|
        word_width = word.size
        combined_width = cell_line_width + word_width
        if combined_width - 1 == maxwidth && word[-1] == " "
          # do nothing
        elsif combined_width > maxwidth
          new_cell_line(true)
        end
        if word_width >= maxwidth
          word.scan(/\X/).each do |r|
            char = r[0]
            char_width = char.size
            if cell_line_width + char_width > maxwidth
              if char != " "
                new_cell_line(false)
              end
            end
            cell_line << char
            cell_line_width += char_width
          end
        else
          cell_line << word
          cell_line_width += word_width
        end
      end
      cells << cell_line.to_s
      line_index += 1
      cell_line.close
    end
  end
  cells
end

5 lines of code can now be managed at the nested method level, there are no more duplicated lines and the total number of lines in the method has been reduced. In addition, IMHO, it makes the code easier to read.

As you see, in this example, I only need one “sub” method.
Using a closure seems a bit more complicated, due in particular to the existence of the += syntax line.

For the moment, I use the following macro in my real code, but I find it less elegant than a nested method as it is declared outside the main method;-))

macro new_cell_line(check_size)
  content = cell_line.to_s
  if ({{check_size}} && content.strip.size != 0) || !{{check_size}}
    cells << content
    line_index += 1
  end
  cell_line.close
  cell_line_width = 0
  cell_line = String::Builder.new
end

For your case, I think it would be appropriate to use a Proc.

def calculate_cells(formatted_content, maxwidth, wrap_preserve)
  cell_line = String::Builder.new
  cell_line_width = 0
  line_index = 0
  cells = [] of String

  new_cell_line = ->(check_size : Bool) do
    content = cell_line.to_s
    if (check_size && content.strip.size != 0) || !check_size
      cells << content
      line_index += 1
    end
    cell_line.close
    cell_line_width = 0
    cell_line = String::Builder.new
  end

  case wrap_preserve
  when :char
    formatted_content.split(/\n/).flat_map do |substr|
      cell_line = String::Builder.new
      cell_line_width = 0
      substr.scan(/\X/).each do |r|
        char = r[0]
        char_width = char.size
        if cell_line_width + char_width > maxwidth
          new_cell_line.call(false)
        end
        cell_line << char
        cell_line_width += char_width
      end
      cells << cell_line.to_s
      cell_line.close
      line_index += 1
    end
  when :word
    formatted_content.split(/\n/).flat_map do |substr|
      cell_line = String::Builder.new
      cell_line_width = 0
      substr.split(/(?<= |\-|\—|\–⁠)\b/).each do |word|
        word_width = word.size
        combined_width = cell_line_width + word_width
        if combined_width - 1 == maxwidth && word[-1] == " "
          # do nothing
        elsif combined_width > maxwidth
          new_cell_line.call(true)
        end
        if word_width >= maxwidth
          word.scan(/\X/).each do |r|
            char = r[0]
            char_width = char.size
            if cell_line_width + char_width > maxwidth
              if char != " "
                new_cell_line.call(false)
              end
            end
            cell_line << char
            cell_line_width += char_width
          end
        else
          cell_line << word
          cell_line_width += word_width
        end
      end
      cells << cell_line.to_s
      line_index += 1
      cell_line.close
    end
  end
  cells
end

formatted_line = "Crystal language is the best!\nThere is nothing to discuss!"

puts formatted_line
puts calculate_cells(formatted_line, 12, :char)
puts calculate_cells(formatted_line, 12, :word)

I also have a question, compared to my code above, what is the advantage of using nested method?


EDIT: i guess you means nested method following code should work? (use ruby code as example)

def foo
  def bar
    puts x
  end
  x = 100
  bar
end

foo

But it not.

1.rb:3:in `bar': undefined local variable or method `x' for main:Object (NameError)

    puts x
         ^
        from 1.rb:6:in `foo'
        from 1.rb:9:in `<main>'
1 Like

Thanks @zw963, I have adopted the code you suggest: the use of a Proc is finally well suited in this case.
As for the ruby nested methods, there is an interesting discussion in this post.

1 Like