Monday, August 3, 2015

Guarding Errors

I was working on using the new Swift "guard" statement in my code and I ran across an oddity that felt strange to me. So I thought I would discuss it here.

Apple defines the "guard" statement as this:
guard statement is used to transfer program control out of a scope if one or more conditions aren’t met. 
So, as my thinking went, shouldn't I use this to bail out of a method when an error condition occurs?

I started trying to use it in my CloudKit code. But as I worked with it, it began to feel wrong.

For example, here is a method I use to save a record to a private CloudKit database prior to changing it to use the "guard" statement.

1.   func saveRecord(record:CKRecord, completionHandler:((record:CKRecord) -> Void)) {  
2.     privateDB.saveRecord(record) {  
3.       record, error in  
4.       if let error = error {  
5.         print("error loading: \(error)")  
6.         dispatch_async(dispatch_get_main_queue()) {  
7.           self.handleError(error)  
8.         }  
9.         return  
10.      }  
11.      if let record = record {  
12.        completionHandler(record: record)  
13.      }  
14.    }  
15.  }  

For my way of thinking, this method reads from top to bottom fairly well.  The callback to the privateDB.saveRecord method starting on line 2 returns two optionals, a CKRecord and a NSError.  

On line 4 I first check the error, and if one exists I log it, call an internal error handler method and bail out of the callback.  This to me sounded like the exact case the "guard" statement was meant for.

If no error was returned then the code continues on at line 11 by first checking to see if a CKRecord was returned in the record parameter and if so it calls the passed in completionHandler. 

Next I rewrote the method using the "guard" statement.  Here is how it looked after my first attempt:

1.   func saveRecord(record:CKRecord, completionHandler:((record:CKRecord) -> Void)) {  
2.     privateDB.saveRecord(record) {  
3.       record, error in  
4.       guard (error == nil) else {  
5.         print("error loading: \(error)")  
6.         dispatch_async(dispatch_get_main_queue()) {  
7.           self.handleError(error!)  
8.         }  
9.         return  
10.      }  
11.      guard (record != nil) else {  
12.        print("record not returned from save")  
13.        return  
14.      }  
15.      completionHandler(record: record)  
16.    }  
17.   }  

Hmm, the method was now 2 lines longer.  But more importantly on line 4, the first use of the "guard" statement, I was confused.  If the condition for the "guard" is true we want to skip the block and continue with the code after the "else" block, if it is false we want to go into the "guard's" "else" block and perform the code there.  

But in this code the existence of the error object is generally considered a bad thing.  Also in my brain (I come from a very Java heavy background) if something is equal to nil then I don't generally think of that as a good condition.

But, it seems to me, by using the "guard" statement my way of thinking about good and bad conditions in a code block is reversed.  Now "error == nil" is a "good thing", but it doesn't mean jump into the next block as that block is there for a "bad thing" or the false condition.

It seems reversed to me.

It gets worse on line 11 where the definition of a "good thing" and a "bad thing" is reversed.  Now if the "record" parameter is not nil we have a good condition and we should skip the "else" block.  Otherwise if the "record" parameter is nil we have a bad condition and we should execute the "else" block.  This is the exact reverse of the conditions in lines 4 through 10.

Yes, I'll admit, if you think about it, both are correct and the context is different in the two "guard" statements.

But I'm a lazy programmer and I tend to scan code quickly.  I like to have repeatable patterns that when I see them I know generally how it will work (without having to be overly concerned about the context at each line of code).

For example if I see a pattern like this:

   if (something == null) {
     // do something
   }
   else {
     // do something else
   }

I expect the true case to follow the condition and the false case to follow the "else".  But the "guard" statement reverses that.

I'm still thinking about this but for now I decided to go back to my original code.

I can see where the "guard" statement would work well in a method that took a few parameters and you could not execute the method unless each of the parameters met certain conditions.  In that use case I think bailing out of the method early is preferable and the "guard" statement facilitates that well.

But for this particular use case I'm not convinced it is a better construct. This could just be a case where I need to get used to using "guard" before it will feel natural.

As always, please don't hesitate to post a comment on your thoughts on using "guard" or where I might be missing the mark.

No comments:

Post a Comment