Best Practice: Try vs Rescue
Try and rescue serve different purposes. The purpose of try
is to save you from having to do:
if user && user.email
Or any situation where the parent object can possibly be nil, which would cause a NoMethodError on NilClass. The purpose of rescue
is to handle exceptions that get thrown by your method invocation. If you expect an exception from calling user.email
, then you can rescue nil
it to prevent the exception from bubbling up.
In general, I'd say avoid using rescue nil
unless you know explicitly what exceptions you are rescuing because you could be rescuing a different exception, and you would never know it because rescue nil
would prevent you from seeing it. At the very least maybe you could log it:
begin
...some code...
rescue => ex
logger.error ex.message
end
Ruby rescue and best practice syntax
The usual rule of thumb is that exceptions should be reserved for exceptional circumstances, those that you don't expect in the normal flow of control. For one thing, they're usually slower than the alternatives.
Here's what I prefer for your scenario:
foo = SomeActiveRecordModel.find_by_bar(10).try(:foo) || ''
Am I abusing rescue for nil checks?
I think you are abusing rescue a bit, though in Rails, there is a specific method for these issues: try
. Documentation
In your case, item.user.try(:name)
might be a nicer approach.
Is it good practice to use exceptions for control flow in Ruby or Ruby on Rails?
Rails is in no way consistent in its use of exceptions. find
will raise an exception if no object is found, but for saving you can choose what behaviour you want. The most common form is this:
if something.save
# formulate a reply
else
# formulate an error reply, or redirect back to a form, or whatever
end
i.e. save
returns true or false. But there is also save!
which raises an exception (adding an exclamation mark to the end of a method name is a Rubyism for showing that a method is "dangerous", or destructive, or simply that it has side-effects, the exact meaning depends on the context).
There is a valid reason for why find
raises an exception, though: if a RecordNotFound
exception bubbles up to the top level it will trigger the rendering of a 404 page. Since you usually don't catch these exceptions manually (it's rare that you see a rescue ActiveRecord::RecordNotFound
in a Rails app), you get this feature for free. In some cases though, you want to do something when an object does not exist, and in those cases you have to catch the exception.
I don't think that the term "best practice" actually means anything, but it is my experience that exceptions aren't used for control of flow in Ruby anymore than in Java or any other language I have used. Given that Ruby doesn't have checked exceptions, you deal with exceptions much less in general.
In the end it's down to interpretation. Since the most common use case for find
is retrieving an object to display it, and that the URL for that object will have been generated by the application, it may very well be an exceptional circumstance that the object cannot be found. It means that either the application is generating links to objects that don't exist, or that the user has manually edited the URL. It can also be the case that the object has been removed, but a link to it still exist in a cache, or via a search engine, I would say that that too is an exceptional circumstance.
That argument applies to find
when used as in your example, i.e. with an ID. There are other forms of find
(including the many find_by_*
variants) that actually search, and those don't raise exceptions (and then there is where
in Rails 3, which replaces many of the uses of find
in Rails 2).
I don't mean to say that using exceptions as control of flow is a good thing to do, just that it's not necessarily wrong that find
raises exceptions, and that your particular use case is not the common case.
Rails 'service objects' best practice - class method or instantiate
In Ruby, I don't find that there's much of a difference.
I find the use of class variables in your "static" version a bit disturbing.
I think the class version might lead to more-creative re-use through subclassing, but that brings its own set of headaches unless things are designed as correctly as possible.
Exception Handling - is this good practice?
else if (min < 0f || max < 0f)
throw new WrongElementValueException("Min/max must be greater than 0!");
else if (min > 100f || max > 100f)
throw new WrongElementValueException("Min/max must be lesser than 0!");
I'd note that there is already ArgumentOutOfRangeException
that's defined for precisely this sort of case.
if (min >= max)
throw new WrongElementValueException("Min must be greater than max!");
This should definitely be an ArgumentException
, but if WrongElementValueException
inherits from ArgumentException
, then that's fine.
Your general approach is sound. I'd consider going further:
HasValue = true;
Why allow for the class to ever not have a value. Consider if you add:
public Element(float min, float max)
{
SetRange(min, max);
}
Now you can never have an instance without its values set, and can get rid of HasValue
entirely.
Note though that I changed this from float?
to float
. You might well be advised to do that throughout the class. Otherwise if you have a need for cases where Min
and Max
are null, (and therefore don't want to get rid of HasValue
) you need to catch that case in SetRange
:
public void SetRange(float? min, float? max)
{
if (min < 0f || max < 0f || min > 100f || max > 100f)
throw new ArgumentOutOfRangeException();
if (min >= max)
throw new WrongElementValueException("Min must be greater than max!");
Min = min;
Max = max;
if(min.HasValue && max.HasValue)
{
Average = (min + max)/2f;
HasValue = true;
}
else
{
Average = null;
HasValue = false;
}
}
(I'd also generally favour double
and double?
over float
and float?
unless you've a strong reason otherwise).
Incidentally, we generally use "exception handling" to talk about how code that's using this code deals with the fact that your code threw an exception, with just what the best thing to do is depending on the context of that code rather than this code.
Is try catch in actors always bad practice?
It's perfectly fine to catch exceptions and make them into responses when you feel it's appropriate. I'd recommend using Scala's Try, as in:
Try(dangerousOperation()) match {
case Success(res) => sender() ! res
case Failure(ex) => sender() ! UnableToStoreThingy("reasons...", ex)
}
Or something like that (you can also try.failed.map { ex => doThings(ex) }
), depends on your style preference.
DRY-ing up begin-rescue-end
DRYing this up is a great idea and a good lesson in rails design (and Test Driven Development/TDD) if you can do so.
Ideally, you'd do something like this:
def create
...
grid = Tag.find_by_id(story[:tag_id]) or raise GridNotFoundError
...
event = Event.find_by_id(story[:event_id]) or raise EventNotFoundError
...
rescue GridNotFoundError
flash.now[:error] = "Please select a category"
process_grid_not_found(item, story, race, etc...)
rescue EventNotFoundError
flash.now[:error] = "Please select an event or create a new one if you don't find your event"
process_event_not_found(item, story, race, etc...)
rescue CreateEventError
flash.now[:error] = "There has been a problem creating your event."
process_event_create_error(item, story, race, etc...)
rescue ActiveRecord::RecordNotSaved, ActiveRecord::RecordInvalid, ActiveRecord::RecordNotFound
flash.now[:error] = item.errors.full_messages.join(" ,")
process_other_error(item, story, race, etc...)
end
render 'home/race_updates'
Then you should create the relavent new methods (process_event_not_found
, etc) as separate (probably private
) methods in the model.
This both makes the code much more readable, but has the great advantage be being much easier to write test code for.
So then you should write test code (using Test::Unit
or rspec
or whatever) that tests the isolated functionality required by each of the individual exception methods. What you'll find is that this both yields better code, as well as it also will likely break-down the exception methods into smaller, more modular methods themselves.
When you hear Ruby and Rails developers talk about the benefits of Test Driven Development, one of the main benefits of that approach is that it is much less likely to result in long, complex methods like the one you've got here. It's much more likely that you'll have code that is much DRYer, with smaller, simpler methods.
I'd also recommend that once you get through this you take another look and try to simplify it further. There will be more room for simplification, but I'd recommend refactoring it iteratively and starting with breaking it down as I've described and getting tests in place to start.
Related Topics
How to Write a Method That Counts the Most Common Substring in a String in Ruby
Sorting of 2D Array by Its Amount of in the Inner Elements
Error When Pushing to Heroku - ...Appear in Group - Ruby on Rails
Error: Missing Rvm Environment File After Doing Rvm Upgrade Command - Passenger 4.0.23
Why Does the Script Affect Everything on My Rails 3 App Even When Cased in This Code
How to Render the Ajax Response in Rails
Deleting Blank Lines After Loop
Replacing an Element in Nested Array Ruby
How to Find the Average of 3 Date in Ruby on Rails or Ruby
How to Keep Sending Emails to Users Every Week Depending on User Date Input in Rails
How to Implement Injection in Ruby
What's the Differences Between Ruby on Rails and Ruby
What Ruby and Rails Developers Ought to Know