Do you treat silent rescue as a code smell?

[DoubleX]

Silent rescue here refers to rescuing without any truly helpful notification at all.

I personally do treat it as a code smell in general(even though I won't claim it's just an outright anti pattern). I think it'll easily and probably lead to sweeping errors under the carpet even when they're better to be at least implicitly exposed. The below are some examples that triggered my alarm:

The default RMVXA saving

Spoiler

Consider save_game under DataManager:

#-------------------------------------------------------------------------- # * Execute Save #-------------------------------------------------------------------------- def self.save_game(index) begin save_game_without_rescue(index) rescue delete_save_file(index) false end end

When errors are raised, this method will delete the save file at the currently selected save slot and return false to indicate the saving's failed.

The players/developers will know that the saving's failed as the buzzer sound meaning failed operations will be played, and the currently selected save slot becomes/remains empty.

Strictly speaking, that's not completely silent, but those not being familiar with how the default RMVXA saving works will have almost no idea of why the saving failed.

Of course, most experienced and/or proficient scripters will immediately come up with the possibility(as it's extremely likely the cause in most cases) that some parts of the current data to be saved contains some objects that can't be serialized(like Bitmap, Fiber, Proc, Sprite, etc) as the default RMVXA saving uses Marshal. Then they can just comment out the rescue parts to figure out what classes are involved in the serialization failures.

In this case, I think that the rescue's sweeping the causes of saving failures under the carpet.

Of course, I think this decision does have its own rationale: It's quite hard to expose the causes without letting the game crash, and once the game's allowed to crash, all the progress between the latest save and the current attempted save will be lost, which can be devastating to some players/developers. Rescuing the errors instead will let the game to continue to progress to hope that the saving will eventually succeed before the game's terminated.

(However, I honestly still don't have a clue on why the save file of the currently selected save slot needs to be deleted if errors are raised.)

Edit: Ramiro(#3) pointed out that the moment the saving attempt started, the old save contents are already gone regardless of whether the attempt succeed, that's why it's better to erase the old save file when the attempt failed.

If, on the other hand, the actual saving part's rewritten from this:

Spoiler

#-------------------------------------------------------------------------- # * Create Save Contents #-------------------------------------------------------------------------- def self.make_save_contents contents = {} contents[:system] = $game_system contents[:timer] = $game_timer contents[:message] = $game_message contents[:switches] = $game_switches contents[:variables] = $game_variables contents[:self_switches] = $game_self_switches contents[:actors] = $game_actors contents[arty] = $game_party contents[:troop] = $game_troop contents[:map] = $game_map contents[layer] = $game_player contents end

into this:

Spoiler

#-------------------------------------------------------------------------- # * Create Save Contents #-------------------------------------------------------------------------- def self.make_save_contents conts = {} [:system, :timer, :message, :switches, :variables, :self_switches, :actors, arty, :troop, :map, layer].each { |cont| # Shows all save file parts being the causes of serialization failures begin conts[cont] = Marshal.load(Marshal.dump(eval("$game_#{cont.id2name}"))) rescue p("$game_#{cont.id2name} contains objects that can't be serialized.") end # } conts end

(File.open and msgbox_p() are excellent alternates to p(), but I think msgbox_p can be too intrusive to players/developers, and many players/developers seldom check the generated files right after the issues occured so those files usually become less helpful than they're supposed to be)

Now whenever a saving failure's due to no Marshal dump for classes that can't be serialized, the corresponding error logs will be immediately generated to pinpoint the save file parts containing objects belonging to such classes. Now those not capable enough to troubleshoot the issues themselves can post more useful reports, and those capable enough will immediately know which parts they'll have to inspect further.

On a side note: There could be some other causes of saving failures and the above modification just handles the most frequent and common one. Whether the other causes need to be handled depends on the actual cases.


The default RMVXA damage formula evaluation

Spoiler

Consider eval under RPG::UsableItem:amage:

def eval(a, b, v) [Kernel.eval(@formula), 0].max * sign rescue 0end

If evaluating the damage formula will crash the game, the errors will be rescued instead of causing the game to indeed crash.

The players/developers will know that the evaluated damage formula has issues if the result's 0 even when it should be impossible.

However, in cases where the damage formula can legitimately return 0 and that damage formula actually has some issues, the players/developers might miss the fact that the damage formula has issues when they're triggered at moments where the damage formula can legitimately return 0.

In this case, I think that the rescue's sometimes sweeping the very existence of damage formula evaluation errors under the carpet.

If, on the other hand, that method's rewritten into this:

Spoiler

def eval(a, b, v) # Indicate the damage formula evaluation has raised errors begin [Kernel.eval(@formula), 0].max * sign rescue p("The damage formula of #{self.class} #{@name} has raised errors") 0 end #end

(File.open and msgbox_p() are excellent alternates to p(), but I think msgbox_p can be too intrusive to players/developers, and many players/developers seldom check the generated files right after the issues occured so those files usually become less helpful than they're supposed to be)

Now players/developers can at least know whether it's a legitimate result when the damage formula returned 0 by checking whether the error logs are shown. This helps them to discover issues earlier. Those being familiar enough with damage formulas can fix the problematic ones themselves, while the others can post those damage formulae in their reports and possibly the entire demo if they're called for.


The default RMVXA variable operation

Spoiler

Consider operate_variable under Game_Interpreter:

#-------------------------------------------------------------------------- # * Execute Variable Operation #-------------------------------------------------------------------------- def operate_variable(variable_id, operation_type, value) begin case operation_type when 0 # Set $game_variables[variable_id] = value when 1 # Add $game_variables[variable_id] += value when 2 # Sub $game_variables[variable_id] -= value when 3 # Mul $game_variables[variable_id] *= value when 4 # Div $game_variables[variable_id] /= value when 5 # Mod $game_variables[variable_id] %= value end rescue $game_variables[variable_id] = 0 end end

If invalid operations come into play, errors will be raised and this method will rescue them by setting the value of the accessed variable as 0.

I don't want to be harsh nor rude, but I really feel that it's merely incomprehensible insanity. To me, there's nearly no way to even tell something does go wrong there unless the value of any accessed variable is always immediately checked after the operations(even then there could be cases where the result can be legitimately 0 but in fact errors are raised and immediately rescued afterwards instead).

In this case, I think the rescue's sweeping the very existence of the variable operation errors under the carpet.

If, on the other hand, this method's rewritten into this:

Spoiler

#-------------------------------------------------------------------------- # * Execute Variable Operation #-------------------------------------------------------------------------- def operate_variable(variable_id, operation_type, value) # Shows which operation's invalid for which 2 objects case operation_type when 0 # Set $game_variables[variable_id] = value # Assignments are always valid when 1 # Add begin $game_variables[variable_id] += value rescue p("Invalid add for #{$game_variables[variable_id]} and #{value}") end when 2 # Sub begin $game_variables[variable_id] -= value rescue p("Invalid subtract for #{$game_variables[variable_id]} and #{value}") end when 3 # Mul begin $game_variables[variable_id] *= value rescue p("Invalid multiply for #{$game_variables[variable_id]} and #{value}") end when 4 # Div begin $game_variables[variable_id] /= value rescue p("Invalid divide for #{$game_variables[variable_id]} and #{value}") end when 5 # Mod begin $game_variables[variable_id] %= value rescue p("Invalid modulus for #{$game_variables[variable_id]} and #{value}") end end # end

(File.open and msgbox_p() are excellent alternates to p(), but I think msgbox_p can be too intrusive to players/developers, and many players/developers seldom check the generated files right after the issues occured so those files usually become less helpful than they're supposed to be)

Now players/developers can at least know something does go wrong during variable operations if they always keep the console open and watch for potential error logs, which can help those knowing what they mean by making troubleshooting easier and simpler, and those not knowing what they mean to cite them when asking for external helps.


What do you think about silent rescue? Do you treat it as a code smell or just outright an anti pattern? Do you instead feel/think that it's completely okay? Feel free to share your views on silent rescue and/or correct me if I've made any mistake

 

[Hudell]

I would rather get the thrown exception.

 

[Ramiro]

 I honestly still don't have a clue on why the save file of the currently selected save slot needs to be deleted if errors are raised.

Quite simple, if the error happens like this:

File.open('save1.rvdata2', 'wb') { |f|   # Wild error appears! Go console!}The save file is already lost anyway, so why should you keep the useless file ?

Opening a file in 'wb' mode "cleans" (or truncates) the file and then fills with new content.

 

[DoubleX]

First, I'll have to criticize myself for making 1 ridiculously severe mistake.

These error logs are meaningless and useless:

Spoiler

#-------------------------------------------------------------------------- # * Execute Variable Operation #-------------------------------------------------------------------------- def operate_variable(variable_id, operation_type, value) begin case operation_type when 0 # Set $game_variables[variable_id] = value when 1 # Add $game_variables[variable_id] += value when 2 # Sub $game_variables[variable_id] -= value when 3 # Mul $game_variables[variable_id] *= value when 4 # Div $game_variables[variable_id] /= value when 5 # Mod $game_variables[variable_id] %= value end rescue $game_variables[variable_id] = 0 end end

While these are significantly better(it'd be even better if they can show which event uses this command but I don't know how to do it yet without major refactoring):

Spoiler

#-------------------------------------------------------------------------- # * Execute Variable Operation #-------------------------------------------------------------------------- def operate_variable(variable_id, operation_type, value) # Shows which operation's invalid for which 2 objects case operation_type when 0 # Set $game_variables[variable_id] = value # Assignments are always valid when 1 # Add begin $game_variables[variable_id] += value rescue p("The addition on the variable with id #{variable_id} is invalid " + "as it tries to add #{$game_variables[variable_id]} by #{value}.") end when 2 # Sub begin $game_variables[variable_id] -= value rescue p("The subtraction on the variable with id #{variable_id} is invalid " + "as it tries to subtract #{$game_variables[variable_id]} by #{value}.") end when 3 # Mul begin $game_variables[variable_id] *= value rescue p("The multiplication on the variable with id #{variable_id} is invalid " + "as it tries to multiply #{$game_variables[variable_id]} by #{value}.") end when 4 # Div begin $game_variables[variable_id] /= value rescue p("The multiplication on the variable with id #{variable_id} is invalid " + "as it tries to divide #{$game_variables[variable_id]} by #{value}.") end when 5 # Mod begin $game_variables[variable_id] %= value rescue p("The modulo on the variable with id #{variable_id} is invalid " + "as the dividend is #{$game_variables[variable_id]} and the divisor is #{value}.") end end # end

I would rather get the thrown exception.

So, for example, the rescue for saving can be turned into something like these:

Spoiler

#-------------------------------------------------------------------------- # * Execute Save #-------------------------------------------------------------------------- def self.save_game(index) begin save_game_without_rescue(index) rescue Exception => error p("The below errors are raised when trying the save at the #{index + 1}th slot:") p(error.message) delete_save_file(index) false end end

Spoiler

  #--------------------------------------------------------------------------  # * Create Save Contents  #--------------------------------------------------------------------------  def self.make_save_contents    conts = {}    [:system, :timer, :message, :switches, :variables, :self_switches, :actors,      :party, :troop, :map, :player].each { |cont|      # Shows all save file parts being the causes of serialization failures      begin        conts[cont] = Marshal.load(Marshal.dump(eval("$game_#{cont.id2name}")))      rescue Exception => error        p("$game_#{cont.id2name} contains objects that can't be serialized:")        p(error.message)      end      #    }    conts  end

And the rescue for damage formula evaluation can be turned into something like this:

Spoiler

def eval(a, b, v) # Indicate the damage formula evaluation has raised errors begin [Kernel.eval(@formula), 0].max * sign rescue Exception => error p("The damage formula of #{self.class} #{@name} has raised the below errors:") p(error.message) 0 end #end

Quite simple, if the error happens like this:

File.open('save1.rvdata2', 'wb') { |f|   # Wild error appears! Go console!}The save file is already lost anyway, so why should you keep the useless file ?

Opening a file in 'wb' mode "cleans" (or truncates) the file and then fills with new content.

I've checked and it turns out that you're right. That means smart players will want to backup their saves to prevent them from being erased in case these unfortunate disasters come, and smart developers will want to advise their players to do so

 

[KockaAdmiralac]

Well, RMVXA cannot crash unless you do anything with scripts ( it can, by deleting resources that are used by some maps or events, or trying to play movie with incorrect format ).

It does not log anything to the console, it does not throw any msgbox_p or msgbox-es ( unless you do something with resources, as noted above ).

So I guess that's why they are sweeping errors under the carpet.

To make it look cleaner.

And I remember when my friend had a problem with saving, I had to comment out that Save rescue ( to realize the problem was the achievment script ).

 

[Ramiro]

I've checked and it turns out that you're right. That means smart players will want to backup their saves to prevent them from being erased in case these unfortunate disasters come, and smart developers will want to advise their players to do so

You could copy a file before saving it and in case of failure, just rename the old file say "you couldn't save properly" and you won't loose any content.

Loosing your game file save is extremely rare anyway, unless you change the game

 

[DoubleX]

You could copy a file before saving it and in case of failure, just rename the old file say "you couldn't save properly" and you won't loose any content.

Loosing your game file save is extremely rare anyway, unless you change the game

I've been thinking about something like this:

Spoiler

#-------------------------------------------------------------------------- # * Execute Save #-------------------------------------------------------------------------- def self.save_game(index) # Makes a temporary save to check if the saving will fail first begin save_game_without_rescue(savefile_max + 1) rescue Exception => error p("The trial saving raised the below error:") p(error.message) return false end # delete_save_file(savefile_max + 1) # Deletes the old save in case the saving failed after a successful trial begin save_game_without_rescue(index) rescue Exception => error p("The saving raised the below error and the old save's deleted:") p(error.message) delete_save_file(index) false end # end

The rationale is that, if the trial saving succeed, it's extremely unlikely that the real saving will fail. So if the trial failed, the old save's still safe; If the trial succeed, it's highly probable that the old save will be replaced by the new one that's legitimate.

Of course this code's far from clean lol

Well, RMVXA cannot crash unless you do anything with scripts ( it can, by deleting resources that are used by some maps or events, or trying to play movie with incorrect format ).

It does not log anything to the console, it does not throw any msgbox_p or msgbox-es ( unless you do something with resources, as noted above ).

So I guess that's why they are sweeping errors under the carpet.

To make it look cleaner.

And I remember when my friend had a problem with saving, I had to comment out that Save rescue ( to realize the problem was the achievment script ).

It can, like sometimes some users screwed up some damage formulae even without using any other custom scripts nor script calls