complete solution(Shabbir Saifee)#37
complete solution(Shabbir Saifee)#37shabbirsaifee92 wants to merge 1 commit intopaircolumbus:masterfrom
Conversation
| if @url.destroy | ||
| redirect_to urls_path | ||
| else | ||
|
|
There was a problem hiding this comment.
destroy doesn't return true or false to tell you whether it succeeded, like some other ActiveRecord methods. So, just redirect unconditionally.
There was a problem hiding this comment.
Oh Thanks. Is it better if implement the error handling here? because, if destroy fails due to some DB issue it will raise an exception right?
There was a problem hiding this comment.
Great question. The destroy call can raise an error for many various reasons. I would not handle those errors in app code as long as we don't expect them from regular app usage. Make sure the app is connected with some error aggregation service, then let "exceptional" exceptions go uncaught.
If we start seeing errors we didn't account for, that's when we should do what we can to prevent them and also rescue them to return a more user-friendly response to our user.
| puts "inside hit_url" | ||
| url = Url.find_by short_url: params[:short_url] | ||
| if url | ||
| url.update('click_count' => url.click_count + 1) |
There was a problem hiding this comment.
Concurrent clicks will be undercounted. Not that it really matters for this exercise, but I think it's a fun exercise to think about doing atomic updates to the database records.
2 participants
@jaybobo