37ce1520d319fa8095d05aa475951616

In a Rails controller I'm saving a number of model objects at once, but I want to be able to revert changes if there is an error part way through.
What do you think of this approach? 

1
2
3
4
5
6
7
8
9
10
11
12
13
14
15
16
17
18
19
20
21
22
23
24
25
26
27
28
29
30
31
32
33
34
35
36
class Test
  @@tries=0
  attr_reader :num

  def initialize( num )
    @num = num
  end

  def do
    @@tries+=1
    puts "Doing #{@num}"
    @@tries<3
  end

  def undo
    puts "Undoing #{@num}"
  end

end

def transact( objs, actions )
  done = []
  objs.each { |obj|
     done.unshift obj
     if !obj.send actions[0]
       puts "Unrolling"
       done.each { |d| d.send actions[1] }
       break
     end
  }
end

actions = [ :do, :undo ]
objs = (1..5).collect { |n| Test.new n }

transact( objs, actions )

Refactorings

No refactoring yet !

37ce1520d319fa8095d05aa475951616

John Sietsma

August 20, 2008, August 20, 2008 07:25, permalink

1 rating. Login to rate!

This is slightly neater and perhaps more idiomatic Ruby.
I forgot I needed a return code for "if transact...". I don't like repeating the length test, but didn't want the extra line: "ok = done.length==objs.length". Any other ways to do this?

1
2
3
4
5
6
7
8
9
def transact( objs, actions )
  done = []
  objs.each { |obj|
     done.unshift obj
     break unless obj.send actions[0]
  }
  done.each { |d| d.send actions[1] } unless done.length==objs.length
  done.length==objs.length
end
37ce1520d319fa8095d05aa475951616

John Sietsma

August 20, 2008, August 20, 2008 07:37, permalink

1 rating. Login to rate!

Well, this covers it but is less readable.
Hmmm, 2 posts in response to my own code! This is like laughing at your own jokes...

1
2
3
4
5
6
7
8
def transact( objs, actions )
  done = []
  objs.each { |obj|
     done.unshift obj
     break unless obj.send actions[0]
  }
  !( done.each { |d| d.send actions[1] } unless done.length==objs.length )
end
861f311cc4a077c439099d0e5d251e73

Wolfbyte

August 20, 2008, August 20, 2008 08:19, permalink

1 rating. Login to rate!

Even less readable, but how about this one line recursive version. It relies on short-circuit evaluation to get the job done. Being recursive it naturally does the roll/unroll that the iterative solution provides

1
2
3
def transact( objs, actions )
  (objs.empty? or (objs[0].send(actions[0]) and transact(objs[1..-1],actions) )) or (objs[0].send(actions[1]) and false)
end
861f311cc4a077c439099d0e5d251e73

Wolfbyte

August 20, 2008, August 20, 2008 09:01, permalink

1 rating. Login to rate!

This version behaves in much the same way but it is easier to figure out what it is doing.

1
2
3
4
5
6
7
8
9
10
11
12
13
14
def transact( objs, actions )
  return true if objs.empty?
  forward, backward = actions
  head, *tail = objs
  succeeded = false
  if head.send forward
    if transact tail, actions
      succeeded = true
    end
  end
  head.send backward unless succeeded
  succeeded
end
A8d3f35baafdaea851914b17dae9e1fc

Adam

August 20, 2008, August 20, 2008 14:06, permalink

2 ratings. Login to rate! 
1
2
3
4
5
6
7
8
9
10
11
12
require 'transaction/simple/group'

def transact(objects, action)
  group = Transaction::Simple::Group.new(*objects)
  group.start_transaction
  
  if objects.all? { |object| object.send(action) }
    group.commit_transaction
  else
    group.abort_transaction
  end
end

Your refactoring





Format Copy from initial code

or Cancel