I'm writing a lot of if statements

As I get deeper into my app, I find myself writing a lot of "if"
statements.

If a user is logged in I want to highlight her name on various postings
and comments.

If a user is logged in, I want their profile page to say "Your last 10
comments" versus "username's last 10 comments".

A partial is used to display a post. If a post is displayed on a list
screen, link the post title to a post_detail page. If a post is
displayed on a post_detail page, don't link the post title to anything.

I just need a nudge in the right direction. Should I simply be moving
all this "if" code to helper classes? Or am I missing a pretty
standard OO or RoR concept here that would make my code much more solid
and less, well, "iffy".

What I like to do is use the "ternary" operator, which works like this:

<%= (session[:user].id == params[:id])? "Your" : session[:user].username
+ "'s" %> Last 10 Comments

The statement in the ( ) is evaluated. If true, the value to the left of
the : is used. If false, the value to the right.
Works wonders for shortening up code. And no, I don't think you need to
reformat your code. But you could.
If there are formatting tasks you repeat a lot, put it into a helper.
But if you just do it once, leave it alone.

bjhess wrote:

Going throw college learning C and C++ I definitely got hooked on the
ternary operator. Good point.

I have a lot of instances of "do this if". So things like:

<%= "<span class="highlight">" if session[:user_id] == @post.user_id %>

Here's the other wrinkle. If a user is logged in, I'm placing a
highlight span around his posting title. This means I'm doing an if
check above and below the title text. Just adding information to my
original post.

Jason Norris wrote:

bjhess wrote:

As I get deeper into my app, I find myself writing a lot of "if"
statements.

Open an old, boring book from ~10 years ago called /Design Patterns/.

Write tests for every single effect of your if statements.

If you duplicate any if statement, refactor your code to create objects
with polymorphic methods. Run all the tests between each small code
change. Share each new object between the places where the if
statements duplicated. This is the heart of OO.

If a user is logged in, I want their profile page to say "Your last 10
comments" versus "username's last 10 comments".

Start by "hiding" those if statements. Put them inside method calls
that return either Your or 'usernames's.

Then move the if statement itself out to a function that decides - once
and only once - which kind of relationship your application has with
the user. if current_user == @user then FamiliarInterface.new else
SociableInterface.new end. Pass that object around, and put inside it
every if statement that dealt with familiarity with the user.

You may eventually arrive at the Decorator Pattern.

BTW one major problem with Ruby is you won't learn many of the
classical design patterns, such as Prototype or Adapter. Ruby makes
them way too easy. /Design Patterns/ was written with C++ and Smalltalk
examples, so you may need to switch to a harder language to learn them.
:wink:

A partial is used to display a post. If a post is displayed on a list
screen, link the post title to a post_detail page. If a post is
displayed on a post_detail page, don't link the post title to anything.

In this case, pass a :local into the partial that writes the link. The
caller knows which object to pass in. The caller could even decide to
pass in a block closure. Or it could chose from predefined and reusable
objects.

Look up "Construction Encapsulation" - another critical technique that
Ruby makes too easy.

I just need a nudge in the right direction. Should I simply be moving
all this "if" code to helper classes? Or am I missing a pretty
standard OO or RoR concept here that would make my code much more solid
and less, well, "iffy".

Yes. The heart of OO, and the reason OO is a powerful design technique,
is that each time you replace a conditional with polymorphism, you have
a new object that can now replace other conditionals

The great thing about Rails is it already did a lot of this OO
polymorphism for you. You no longer have to claw your way through a
maze of if Database if Controller if View nonsense. These kids these
days probably don't remember classical Perl CGI website development!
Back in my day, we had to bless our hashes!!

The power of Rails has lead you to create a complex beast of your own,
hosted within it. So you now need to apply your own abstractions, to
keep your own layer of the design clean and flexible.

If you don't clean up your design (and if you don't write unit tests)
then each time you add a new feature, you must balance its new if
statements against all your old ones, including ones you might forget
about. This decay will cause bugs, and the design quality will go down.
Then, fixing bugs will cause more bugs.

So the time to learn OO and apply it here is as early as possible,
before the design decays!

Philip, thanks for the reply.

I was thinking about highlighting and threw together this design. In
the end, I don't know that it is useful. But it was a worthwhile
activity. This is almost exactly like Design Patterns' Decorator
pattern. I'm wondering if there is a way to bake the user comparisons
into this somehow.

An abstract TextComponent class with a TextComponent(text) constructor
and a text attribute. The constructor stores the text parameter in the
text attribute. There is also a render method that would simply return
the text attribute.

A ConcreteTextComponent class that extends the abstract.

A HighlightDecorator that extends the TextComponent abstract class. It
would have a HighlightDecorator(TextComponent) constructor. It would
have a text_component attribute. It's render method would look like:

   def render
     "<span class='highlight'>#{text_component.render}</span>"
   end

In the ApplicationHelper class there would be a
print_text(TextComponent) method. It would simply do
text_component.render.

In this way all highlighting in the app would be achieved with the
following call:

print_text(HighlightDecorator.new(ConcreteTextComponent.new("text")))

Not sure if that is an improvement over the inline highlight spanning.

bjhess wrote:

print_text(HighlightDecorator.new(ConcreteTextComponent.new("text")))

Not sure if that is an improvement over the inline highlight spanning.

That is Java-style Ruby. Can't you do a trick with fewer variables, and
a block closure?

I started writing this response earlier, but it makes more sense here anyway :wink:

You can always choose to hide some of those 'if' statements in some helper methods:

# In a helper module, you could put:

def user_possessive(user_id)
   if session[:user].id == user_id
     "Your"
   else
     session[:user].username + "'s"
   end
end

# In your view:

<%= user_possesive(params[:id]) %> Last 10 Comments

# For the optional span tags, perhaps something like this in a helper:

   def highlight_if(condition)
     if condition
       content_tag(:span, yield, :class => 'highlight')
     else
       yield
     end
   end

# which in a view would be used like:

<%= highlight_if(session[:user_id] == @post.user_id) { @post.title } %>

Sorry I didn't Decorate and Componentize it as much 8^)

-Rob

Rob Biedenharn http://agileconsultingllc.com
Rob@AgileConsultingLLC.com

Rob Biedenharn wrote:

Sorry I didn't Decorate and Componentize it as much 8^)

Just so long as you didn't OCP it.

:-]

Phlip wrote:

bjhess wrote:

> print_text(HighlightDecorator.new(ConcreteTextComponent.new("text")))
>
> Not sure if that is an improvement over the inline highlight spanning.

That is Java-style Ruby.

There's definitely a reason for that. I'm a Java-convert trying to
make my way in a Ruby world. :slight_smile:

bjhess wrote:

There's definitely a reason for that. I'm a Java-convert trying to
make my way in a Ruby world. :slight_smile:

Heck - I started with C++ style Ruby. Imagine the horror!

Just for you, I added a simple feature to my "how to Chat" project. I
made my new feature extra-complex, to show what we meant by the
Decorator pattern in Ruby.

I'm not sure if this is true Decorator - that's the great thing about
Ruby; it makes abstractions so easy that you don't need to know if they
are actually design patterns or not. But at least my pattern ...
decorates my chat panel.

This calls the partial. Stare at the last =>:

    <%= render :partial => 'chat/index',
               :locals => {
                 :chat_style => {
                   :panel_id => 2,
                   :panel_height => '10ex',
                   :get_background_color => proc { '#eeeeee' }
                   } }

That sets the :get_background_color parameter to a block. The block
simply returns a string, but it _could_ have returned one branch of
your 'if' statement. Maybe we have complex caller-specific logic to set
the color. If this logic belongs to the caller, then we should not
taint the partial with knowledge about the caller. The caller can put
its own variables into the proc{}, even local ones.

The partial system turns :chat_style into a local variable (per
:locals). Then we can read its parameters out of it:

    panel_id = chat_style[:panel_id]
    panel_height = chat_style[:panel_height] || '30ex'
    get_background_color = chat_style[:get_background_color] || proc {
'#ffffee' }

(There may be other ways to pass locals and then provide defaults...)

I provide a default for :get_background_color, of a different string.
Again, that proc could have contained complex logic to generate the
background color. Consider this the 'else' branch of your 'if'
statement.

Now we finally put our lowly background color generating proc to good
use:

<div
    id='panel_<%= panel_id %>_chat_history'
    style='height: <%= panel_height %>;
          font-size: 90%;
          border-bottom: #eeeeee solid 1px;
          overflow: auto;
          background-color: <%= get_background_color.call %>'
    >

The result is our DIV style string doesn't contain an 'if' _or_ an
'else'. It only contains a call to a polymorphically defined function.
The target functions are defined up in the modules best able to direct
their contents.

Thanks for all the replies. Excellent information here!

Another fairly complicated scenario I have going. On the post detail
screen, numerous things can display three different ways depending on
if the user is the original author, not the original author, or if no
user is logged in.

For instance, at the bottom of the post comments, there is a form to
write a comment. It will only appear if there is a user logged in and
said user is not the post's author. The form will not display if the
logged in user is the post's author. If there is no logged in user? A
"Please register to comment" message will appear.

I've refactored this once into a helper method so that my view now only
has a call to "comment_entry(@user, @post)". At the least, this gives
me some ability to unit test the process.

(Side note: To keep track of the user's author status, I'm setting an
"is_author" attribute on the user instance. What feels wrong about
this is that I'm directly setting this attribute in a controller at the
times that I care about author status.)