Avatar

I know there has to be a "ruby" way to remove the duplicate methods. The logic is the same, just the ActiveRecord attribute changes. I tried a few things but my unit tests failed.

Thanks

1
2
3
4
5
6
7
8
9
10
11
12
13
14
15
16
17
18
19
20
21
22
23
  def planned_percent_complete
    if self.planned_complete_in_dollars != nil
      (( self.planned_complete_in_dollars.to_f / self.task.budget.to_f )  * 100).round(2)
    else
      0
    end
  end

  def planned_percent_complete=(ppc)
    self.planned_complete_in_dollars = (ppc.to_i / 100.0) * self.task.budget
  end

  def actual_percent_complete
    if self.actual_completed_in_dollars != nil
      (( self.actual_completed_in_dollars.to_f / self.task.budget.to_f )  * 100).round(2)
    else
      0
    end
  end

  def actual_percent_complete=(ppc)
    self.actual_complete_in_dollars = (ppc.to_i / 100.0) * self.task.budget
  end

Refactorings

No refactoring yet !

4f0b71df92ecba6051d9524a60be4d93

Derek Perrault

January 8, 2008, January 08, 2008 19:35, permalink

No rating. Login to rate!

My method would require you to change #actual_completed_in_dollars to #actual_complete_in_dollars so it mirrors the #planned_complete_in_dollars name.
Also, I didn't know what to call the method I extracted in the "setters". Since you know the domain, you can give it a good name.

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
def planned_percent_complete
  percent_complete :planned
end

def actual_percent_complete
  percent_complete :actual
end

def planned_percent_complete=(ppc)
  self.planned_complete_in_dollars = good_name_for_this(ppc)
end

def actual_percent_complete=(ppc)
  self.actual_complete_in_dollars = good_name_for_this(ppc)
end

private

def percent_complete(which)
  self.send("#{which}_complete_in_dollars").nil? ? ((self.send("#{which}_complete_in_dollars).to_f / self.task.budget.to_f) * 100).round(2) : 0
end

def good_name_for_this(ppc)
  (ppc.to_i / 100.0) * self.task.budget
end
4f0b71df92ecba6051d9524a60be4d93

Derek Perrault

January 8, 2008, January 08, 2008 19:39, permalink

No rating. Login to rate!

*sigh*
I forgot to close a string. I noticed when it hit the syntax highlighter.
Here it is all fixed up.

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
def planned_percent_complete
  percent_complete :planned
end

def actual_percent_complete
  percent_complete :actual
end

def planned_percent_complete=(ppc)
  self.planned_complete_in_dollars = good_name_for_this(ppc)
end

def actual_percent_complete=(ppc)
  self.actual_complete_in_dollars = good_name_for_this(ppc)
end

private

def percent_complete(which)
  self.send("#{which}_complete_in_dollars").nil? ? ((self.send("#{which}_complete_in_dollars").to_f / self.task.budget.to_f) * 100).round(2) : 0
end

def good_name_for_this(ppc)
  (ppc.to_i / 100.0) * self.task.budget
end
63b22ac9bff0cd55d8a91da4dbf00693

Dan Kubb

January 8, 2008, January 08, 2008 20:06, permalink

1 rating. Login to rate!

Here's an alternate approach that dynamically defines the methods.

1
2
3
4
5
6
7
8
9
10
[ :planned, :actual ].each do |type|
  define_method("#{type}_percent_complete") do
    return 0 if send("#{type}_complete_in_dollars").nil?
    ((send("#{type}_complete_in_dollars").to_f / task.budget.to_f) * 100).round(2)
  end

  define_method("#{type}_percent_complete=") do |ppc|
    send "#{type}_complete_in_dollars=", (ppc.to_i / 100.0) * task.budget
  end
end
4f0b71df92ecba6051d9524a60be4d93

derekperrault.wordpress.com

January 8, 2008, January 08, 2008 20:23, permalink

No rating. Login to rate!

My refactoring above had a bug. The ?: return expressions were out of order. Finally, I extracted the dynamic #send so as to be more dry.
I'm done, really.

(and I couldn't edit until I hooked up my actual login... sorry bout the multiple posts)

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
def planned_percent_complete
  percent_complete :planned
end

def actual_percent_complete
  percent_complete :actual
end

def planned_percent_complete=(ppc)
  self.planned_complete_in_dollars = good_name_for_this(ppc)
end

def actual_percent_complete=(ppc)
  self.actual_complete_in_dollars = good_name_for_this(ppc)
end

private

def percent_complete(which)
  complete_in_dollars(which).nil? ? 0 : (complete_in_dollars(which).to_f / self.task.budget.to_f) * 100).round(2)
end

def good_name_for_this(ppc)
  (ppc.to_i / 100.0) * self.task.budget
end

def complete_in_dollars(which)
  self.send("#{which}_complete_in_dollars")
end
Avatar

Chris W.

January 9, 2008, January 09, 2008 01:39, permalink

No rating. Login to rate!

Here is the final version I ended up with. It was a minor tweak on the dynamic define. I had to create the hash since my method names had some verb tense changes.

1
2
3
4
5
6
7
8
9
10
11
12
13
14
# dynamically define the methods to get/set based on percent complete for both the
# actual and planned values
methods = { :planned => 'complete', :actual => 'completed' }

methods.each do |type, verb|
  define_method("#{type}_percent_complete") do
    return 0 if send("#{type}_#{verb}_in_dollars").nil?
      ((send("#{type}_#{verb}_in_dollars").to_f / task.budget.to_f) * 100).round(2)
  end

  define_method("#{type}_percent_complete=") do |pc|
    send "#{type}_#{verb}_in_dollars=", (pc.to_i / 100.0) * task.budget
  end
end
Avatar

Charles

January 9, 2008, January 09, 2008 06:17, permalink

No rating. Login to rate! 
1
Note that nil.to_f is 0.0, so you can probably remove the special case too

Your refactoring





Format Copy from initial code

or Cancel