Chris Wanstrath makes a good point about an ugly way to initialize a variable, but I don't agree that an explicit return is the best style to use.
The original ugly:
def logger
unless @logger
@logger = Logger.new(STDOUT)
@logger.level = Logger::WARN
end
@logger
end
Chris' improvement:
def logger
return @logger if defined? @logger
@logger = Logger.new(STDOUT)
@logger.level = Logger::WARN
@logger
end
I prefer a functional style:
def logger
@logger || begin
@logger = Logger.new(STDOUT)
@logger.level = Logger::WARN
@logger
end
end
This is actually slightly awkward. I'd usually do @logger ||=, but that's hard to do gracefully since here you have to combine a few statements to create the new logger. A better constructor or a #level method that returned self would be so much nicer. A small refactoring seems a bit more readable.
def logger
@logger ||= default_logger
end
def default_logger
logger = Logger.new(STDOUT)
logger.level = Logger::WARN
logger
end
And if you really like functional style and are a fan of the K combinator:
def logger
@logger ||= Logger.new(STDOUT).tap {|l| l.level = Logger::WARN}
end
I am almost always opposed to using an explicit return in Ruby code. Whenever I see one I cringe. You can sometimes make a case for one as a top guard in a medium-sized method, but here's where I invoke the slippery-slope argument. It's too easy for those things to snowball into a handful of return statements, and then someday you'll have to wonder if they have to be in that order or not. And when your method gets long enough, you can easily miss that there was a return at the top and mess up a refactoring; using an if/then/else makes it clearer and harder to miss.
By the way, using if defined? @logger the way Chris does is not the same thing as if @logger.
>> defined?(@logger)
=> nil
>> @logger
=> nil
>> @logger = nil
=> nil
>> defined?(@logger)
=> "instance-variable"
If you're trying to make sure @logger is not nil, defined? is unlikely to be the way you want to test the variable was initialized. Be careful out there, kids...
UPDATE: Oh awesome, it turns out that Logger.new takes a 2nd arg, the level. So this could all be done with:
def logger
@logger ||= Logger.new(STDOUT, Logger::WARN)
end
Just pretend this whole thing was about some other class where this actually mattered.
Your style is definitely best for memoization.
Now what about explicit "return true" calls in AR callback methods? ;)
You can also do something like
I think the defined? case is more useful when the extact role/type of the value is unknown and thus the assumption that nil/false are acceptable default states is not okay. In the case of a logger I would probably say if @logger since you no a logger can't really be nil or false. In the case of run once and only once then defined? is closer.
As for the general refactoring here, I agree that defaults are nice but a separate method is kind of a drag especially when you start getting more methods in the file to organize (alphabetical doesn't work and category is always arbitrary). I tend to keep methods short enough where one top return is clear.
I will say that returns outside of the top level of a method however are something I really really like to avoid. Most of the cases I know I've done it though either involve Ruby bugs (and there are quite a few in 1.8 that are stuck as part of the language) or heavily optimized code where I knowingly chose the trade.
Why the explicit .tap{ ... } over simply chaining the .level= instead?
The logger.new style seems the most sane here. The overhead of the #tap method and the extra default_logger call is not worth the prettier code in my opinion.
Jason: with
Object#tapwe don't have to type another line to return the logger object (it returnsselfafter evaluating the block).As a side note,
@logger ||= Logger.new(STDOUT, Logger::WARN)is doing a bit different work than@logger || begin # .... If I remember correctly,||=would roughly translate to:I may be wrong with boolean operator precedence (heck, I may just be wrong), but I'm almost certain
defined?is involved. Class variables, for instance:And hashes with default values:
"When your method gets long enough" is never a good basis for an argument about coding style. Don't write long methods and multiple returns won't be hard to understand. On the contrary, they are often the most intention-revealing way to return a value.
Mark, "When your method gets long enough" is a valid basis for thinking about coding style. Don't use a style that doesn't scale up if it doesn't offer significant advantages for the smaller methods. 1-liner explicit return top guards look nice in a small method, but nearly anything is readable in a small method. Using a code style that leads you to bad code later is not sustainable. I've seen far more bad code with top return guards than without, which is telling enough for me.
Having seen bad code is also not a good basis for an argument.
I have seen far more long methods than short ones in bad code, too. God, I have seen functions with over 700 lines. When you choose short methods, once you're down to a one-liner, you're done. When you embrace long methods, the only thing stopping you is your ability to scroll, or how your editor relies on RAM.
Bit I digress. Let's assume we're talking about good-to-awesome code, because bad code is hopeless no matter how you style it.
I can tell you what I see when I have the rare pleasure of running into great code: short methods and early returns.
I meant that as a matter of coding style methods (in general) should not be long.
In times like this, I think it's important to see everything like we're in The Warriors.
You are obviously one of the Iffies. I'm one of the Returnies. We both sympathize with the Lambdies, and I guess nobody really likes the Nesties, but you're more lenient with them, because to be an Iffie you'll need at least one level of nesting.
I'd go on, but I have to return early.
Sorry Josh, not buyin' it...
Guard clauses vs. block conditionals are a matter of preference.
The argument that guard clauses make refactoring harder with long methods or bad code is just a straw man. If you screw up a refactoring, it's because you didn't understand the code or didn't have good enough tests. Not because there was a guard clause.
Actually, I like guard clauses BECAUSE they make it easier to debug and understand logic flow. If I'm reading or debugging a method with a guard clause, I KNOW that if the guard clause is true, NOTHING else in the method will be executed. With a conditional block, I have to read down and make sure there's nothing else after the block that matters - and if there is, I have to pay attention to indentation (if it's a long or 'bad' method).
Also, I hate nested conditionals, and guard clauses can help you avoid these without having to extract another method.
Having said that, I agree that if you have multiple guard clauses, you have to worry about order, and that can get confusing.
-- Chad
The moral to this story is that if some code is an outlier of ugliness, there's probably a vendor source. The solution to this programming "challenge" is to fix the Logger interface. And in any other class where something similar is happening, there's an interface waiting to be fixed there too. Honesty, why would you have a class with attributes that can't be set on initialization, and yet don't depend on a post-initialized state? Am I missing something?
@Floyd +1
(@Chad) Re multiple guard clauses and order, once you have something that depends on order anyway, it's best to eye-parse it linearly than go branching crazy following nested ifthenelsery. Principle of Least Worse?
@Chad +1
I like guard clause because they ensure that the code after them is the 'normal' case. And I love them when they avoid several levels of conditional statements. So I prefer:
to:
Even worse is when you have to set flag variables; for example, to escape from a nested loop.
Philippe, that's just uglified for the sake of making it ugly. Nice strawman, but I don't buy it. That's much better off refactored with some kind of state machine or other extracted methods.
I think Floyd is making sense, and I agree that ugly conditional logic probably means you're using an ugly API/class.
I wonder if my distaste of explicit returns in Ruby is because they are so very rarely needed. Java has no default/implicit return value, so you always need a return statement to supply a value. Smalltalk returns self by default. But Ruby returns whatever the last expression of the method is, so you never actually need one. Maybe I just like the purity of never using them. And at some level they feel like GOTOs, which we all righteously abhor. Is anyone defending explicit returns who hasn't spent a lot of time in Java or C or the like? Where do people acquire their sense of style anyway?
Languages with pattern-matching function definitions like Erlang and Haskell (and SASL, precursor of Haskell, which was about my 3rd high-level language way back when) have excellent syntax for succinctly creating simple cases like one-line guards. If Ruby had that I'd be all for it. As it is, my preference is to avoid the explicit return. YMMV (and clearly it does).
Well, all the suggestions are fine, but for me all that works is good.
Who knew it was such a heated topic?
What about the use of a return (even at the end) to separate visually methods that are supposed to return something from those that aren't? Seems to me that would aid refactoring.
I think Floyd is making sense, and I agree that ugly conditional logic probably means you're using an ugly API/class.
Well I also think Floyd is pretty logical. But nevertheles I am a big fan of the solution of Steven. Simple and effective.