I'm digging through some older code in my application and i stumbled on
this: (variable names omitted for brevity)
def the_method
...some code
if
...some code
else
..some code
end unless condition #->BOTHERS ME
...more code
end #final end
end
The conditional dependent 'end' is irritating me a lot. I don't mind mid
method break or return statements, but this just looks obnoxious. I'm
tempted to re-write the whole method to get rid of it.
But maybe i'm just crazy, and this is perfectly acceptable execution
control.
The conditional dependent 'end' is irritating me a lot. I don't mind mid
method break or return statements, but this just looks obnoxious. I'm
tempted to re-write the whole method to get rid of it.
There is no conditional "end". The "end" belongs to a bigger statement
(which you left out), and *this* statement is conditional.
But this is bad style, anyway. You should use the "unless" modifier only
for one line statements like "puts i unless i < 0". If the statement is
longer, use the "unless" statement:
Your instincts serve you well. The unless at the end is a surprise, a
hidden gotcha sprung on the reader at the last moment that completely
changes their understanding of the logic.
With effectively two levels of conditionals, this code is a good candidate
for a second method. In the extracted method, the extra condition can be
converted to a guard clause:
def the_method
...some code
some_other_method
end
def some_other_method
return unless condition
if
...some code
else
..some code
end
end
···
On Wed, Jun 27, 2012 at 10:23 AM, masta Blasta <lists@ruby-forum.com> wrote:
end unless condition #->BOTHERS ME
...more code
end #final end
The conditional dependent 'end' is irritating me a lot. I don't mind
mid method break or return statements, but this just looks obnoxious.
I'm tempted to re-write the whole method to get rid of it.
If it bothers your brain to look at it, definitely rewrite it so
that your brain feels more comfortable.
I myself use short conditionals usually only in block form.
Example:
def foo
invoke_this_method {
method_one
method_two
} unless @run_application
end
This is not that easy to read for others, and I don't do it often, but
when I do it, I try to make it be the last part of the method.
Having an:
"end unless condition_is_true"
in the middle of a method can be difficult to follow.
I try to keep things as simple as possible usually though,
so I rarely use a construct like this.
def some_other_method
return unless condition
if
...some code
else
..some code
end
end
To be honest, I find this almost as obscure as the original code. A
method doing nothing if a certain condition isn't met? I think the
*method call* should be conditional rather than the method body. In
other words: I'd move the "unless" outside of the method body.
But I'm not even sure if we actually have an "if" inside an "unless",
because mastablasta's indentation suggest the "unless" belongs to
another statement or maybe a block (which we cannot see).
The conditional dependent 'end' is irritating me a lot. I don't mind
mid method break or return statements, but this just looks obnoxious.
I'm tempted to re-write the whole method to get rid of it.
If it bothers your brain to look at it, definitely rewrite it so
that your brain feels more comfortable.
I myself use short conditionals usually only in block form.
Example:
def foo
invoke_this_method {
method_one
method_two
} unless @run_application
end
This is not that easy to read for others, and I don't do it often, but
when I do it, I try to make it be the last part of the method.
Having an:
"end unless condition_is_true"
in the middle of a method can be difficult to follow.
I try to keep things as simple as possible usually though,
so I rarely use a construct like this.
Digging through more code i found an even more convoluted example. Here
there is a double end conditional. One of them is for a block. How can
you conditionally end a block??
Sry for long line width
def rebuild_todo_items(todo_type, notify=false) #add case for when users are suspended to delete all their todo
items.
case
when todo_type == "pending term"
skip_term_versions = Array.new
self.todo_items.of_type("pending term").each do | tdi |
if tdi.primary_object.state_owners.include?(self)
skip_term_versions << tdi.primary_object
else
tdi.unassign_from_owner(self)
end
end
self.term_versions_created.with_state("input_requested").each do |
tv |
tv.build_todo_ownership(self) unless
skip_term_versions.include?(tv)
end
self.term_versions_created.with_state("rejected").each do | tv |
tv.build_todo_ownership(self) unless
skip_term_versions.include?(tv)
end
self.functional_area_memberships.each do | fam |
fa = fam.functional_area
fa.terms.each do | t |
if t.versions.latest.state == "pending" &&
!(skip_term_versions.include?(t.versions.latest))
t.versions.latest.build_todo_ownership(self)
end
end if self.role_for(fa).include?("moderator") ||
self.role_for(fa).include?("manager") #-----------> GOTCHA
end unless self.functional_area_memberships.empty? #---------->
GOTCHA AGAIN?
when todo_type == "report request"
Digging through more code i found an even more convoluted example. Here
there is a double end conditional. One of them is for a block. How can
you conditionally end a block??
You can't, the condition refers to the whole method call. As an example:
···
#------------
condition = true
[1, 2].each do |i|
puts i
end if condition #------------
If the condition is met, then the elements of the array will be printed.
This is the same as wrapping the method call (with the block) into an if
statement:
#------------
condition = true
if condition
[1, 2].each do |i|
puts i
end
end #------------
Since the first variant is obviously hard to read and can be
misunderstood, you should definitely rewrite the code to the second
variant. It may be a bit longer and less "cool", but you can understand
it without playing parser first.
Sry for long line width
Yeah, it would have been better if you'd attached the file.