F7b2fe18bb8705a677051523cf62d30f

This seems like a very long way to get to the result desired.

Any refactorizations are greatly appreciated!

Model

1
2
3
4
5
6
7
8
9
10
11
comment_type = translation.class.to_s == "Project" ? "project" : "header"
if comment = translation.find_comment_by_type(comment_type)
  comment.text += comment
else
  new_comment = Comment.new( :text => comment, :comment_type => comment_type )
  if comment_type == "project"
    translation.comment = new_comment
  else
    translation.comments << new_comment
  end
end

Refactorings

No refactoring yet !

F288a8afe5302a16a366d5e9d34f2fec

Joe Grossberg

August 29, 2008, August 29, 2008 20:34, permalink

No rating. Login to rate!

What is going on, on line 2? Are you sure you don't mean "==" instead of "="?

As it stands now:
* if translation.find_comment_by_type(comment_type) is nil, it will assign comment to nil and it will skip to line 5
* if translation.find_comment_by_type(comment_type) is *not* nil, it will assign that value to comment and it will excecute the code on line 3

I say, comment out your code. Write some (failing) unit tests for a variety of scenarios, and then create code to pass those tests.

I can almost guarantee you that the code isn't doing what you expect.

Also, pay attention to warnings; you should be seeing one like "warning: found = in conditional, should be ==".

A8d3f35baafdaea851914b17dae9e1fc

Adam

August 29, 2008, August 29, 2008 21:03, permalink

No rating. Login to rate!

I couldn't quite discern what exactly you are trying to do. I'm guessing there is a much better way to do it. But based on what I could gather:

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
37
38
39
class Commentor
  def comment_type
    self.class.to_s.downcase
  end
  
  def comment
    comments.find_by_comment_type(comment_type)
  end
  
  def build_comment(*args)
    comments.build(*args)
  end
  
  def add_comment(text)
    if comment
      comment.text += text
    else
      build_comment(:text => text, :comment_type => comment_type)
    end
  end
end

class Header < ActiveRecord::Base
  include Commentor
  has_many :comments
end

class Project < ActiveRecord::Base
  include Commentor
  has_one :comment
end

class Comment < ActiveRecord::Base
  belongs_to :header
  belongs_to :traslation
end

# Usage:
# translation.add_comment("This is a comment")
A8d3f35baafdaea851914b17dae9e1fc

Adam

August 29, 2008, August 29, 2008 21:11, permalink

No rating. Login to rate!

Edit: Double post.

F7b2fe18bb8705a677051523cf62d30f

vrish88

August 29, 2008, August 29, 2008 21:42, permalink

No rating. Login to rate!

Sorry for no explanation earlier, but here is a brief one:

This block of code is in a module that is used by two separate classes, "Project" and "Translation". Comment is a polymorphic class that has different comment types (ie project, header, translator, etc). It is also kinda like STI but there is no inheritance; there are just comments with different types.

Projects only have one comment, while translations have many comments. But I want translations to only have one comment with comment_type "header". So here's some pseudo code:

comment_type = assign "project" if the object that is calling this module is a Project otherwise assign "header" for a translation object

if this object already has a comment with the type assigned to comment_type assign that comment to "existing_comment"
add the text to the existing comment
else # a "header" or "project" type of comment has not been created for this object
create comment and add it to the object

So the part that I think can be refactored is within the else block. Because of the has_many or has_one relationship the object (whether it be a Project or a Translation) needs a different operator.

Also I changed the variable that was being assigned in the if statement to "existing_comment" from just "comment". I realized that I had already had a variable named comment earlier in my code. This variable "comment" holds the text that is to be assigned to the new or existing comment for the object (either a Project or Translation)

translation.rb

1
2
3
class Translation < ActiveRecord::Base
  include parser.rb
  has_many :comments, :as => :resource, :dependent => :destroy

project.rb

1
2
3
class Project < ActiveRecord::Base
  include parser.rb
  has_one :comment, :as => :resource

comment.rb

1
2
class Comment < ActiveRecord::Base
  belongs_to :resource, :polymorphic => true

Module parser.rb

1
2
3
4
5
6
7
8
9
10
11
comment_type = translation.class.to_s == "Project" ? "project" : "header"
if existing_comment = translation.find_comment_by_type(comment_type)
  existing_comment.text += comment
else
  new_comment = Comment.new( :text => comment, :comment_type => comment_type )
  if comment_type == "project"
    translation.comment = new_comment
  else
    translation.comments << new_comment
  end
end

Your refactoring





Format Copy from initial code

or Cancel