My recursive function does not end. What am I doing wrong?

Question

I have a model called folder that acts as a tree. Inside the model I have an instance method copy that copies folders from one place to another. When copying a folder, its sub-folders also have to be copied.

This is my code:

class Folder < ActiveRecord::Base
  acts_as_tree :order => 'name'
  before_save :check_for_parent

  def copy(target_folder)
    new_folder = self.clone
    new_folder.parent = target_folder
    new_folder.save!

    # Copy sub-folders recursively
    self.children.each do |folder|
      folder.copy(new_folder) unless folder == new_folder
    end
  end

  def check_for_parent
    raise 'Folders must have a parent.' if parent.nil? && name != 'Root folder'
  end
end

Now consider the following situation:

Root folder-+
            |
         Folder 1-+
                  |
               Folder 2-+
                        |
                      Folder 3

When I copy Folder 1 in the root folder it works fine. It also works when I copy Folder 1 into Folder 2, but when I copy Folder 1 into Folder 3 I end up with endless recursion. In code:

f1 = Folder.find_by_name('Folder 1')
f3 = Folder.find_by_name('Folder 3')
f1.copy(f3) # Never stops

This code leads to:

Root folder-+
            |
         Folder 1-+
                  |
               Folder 2-+
                        |
                      Folder 3-+
                               |
                            Folder 1-+
                                     |
                                  Folder 2-+
                                           |
                                         Folder 3-+
                                                  |
                                               Folder 1-+
                                                        |
                                                     Folder 2-+
                                                              |
                                                            Folder 3-+
                                                                     |
                                                                  Folder 1-+
                                                                           |
                                                                          Etc.

I am overlooking something trivial but I just can't figure it out. What am I doing wrong??

score 1 · Answer 1 · answered Apr 17 '11 at 23:51

Try changing the order of your method so that it reaches the leaf node first, before doing your recursion:

def copy(target_folder)
  new_folder = self.clone

  # Copy sub-folders recursively
  self.children.each do |folder|
    folder.copy(new_folder) unless folder == new_folder
  end

  new_folder.parent = target_folder
  new_folder.save!
end

Your problem is that you're starting by reparenting 'Folder 1' under 'Folder 3'. Then your recursive call runs. When it gets to 'Folder 3' it now has 'Folder 1' as a child, and the cycle continues.

Thanks. Yes, I tried that already, but in that case I get an error because of the `before_save`. Should have made that more clear in my question probably. — Mischa, Apr 17 '11 at 23:55

score 0 · Answer 2 · answered Apr 17 '11 at 23:33

0

Try doing the copying of the current folder before loop that implements the recursion (i.e. pre-order recursion).

answered Apr 17 '11 at 23:33

Marcin

48,559
18
128
201

What do you mean exactly? It seems I am already doing the copying of the current folder before the loop. (In the first three lines of the method) Could you show me some code? Thank you! – Mischa Apr 17 '11 at 23:43

Mischa · Accepted Answer · 2014-08-28T02:47:11.207

I had to keep track of which folder I originally copied. The code below works. Of course, if anyone sees room for improvement, please let me know.

def copy(target_folder, originally_copied_folder = nil)
    new_folder = self.clone
    new_folder.parent = target_folder
    new_folder.save!

    originally_copied_folder = new_folder if originally_copied_folder.nil?

    # Copy sub-folders recursively
    self.children.each do |folder|
      folder.copy(new_folder, originally_copied_folder) unless folder == originally_copied_folder
    end
  end

My recursive function does not end. What am I doing wrong?

3 Answers3