HiveBrain v1.2.0
Get Started
← Back to all entries
patternswiftMinor

Safe init using failable guarded initializer

Submitted by: @import:stackexchange-codereview··
0
Viewed 0 times
initusingfailablesafeguardedinitializer

Problem

I am wondering if that is a safe and proper init, with one failable init guarded and an init.

Are the force-unwrap correct in the self.init as I know they are those types ?

struct Item: Equatable {

            let id: NSNumber 
            let displayName: String
            let amount: NSNumber

            init(id:NSNumber, displayName:String, amount:NSNumber) {
                self.id = id
                self.displayName = displayName
                self.amount = amount
            }

            init?(dictionnary:NSDictionary) {

                guard let id = dictionnary["id"] , id is NSNumber else  {
                    return nil
                }

                guard let displayName = dictionnary["display_name"] , displayName is String else {
                    return nil
                }

                guard let amount = dictionnary["amount"] , amount is NSNumber else  {
                    return nil
                }

                    self.init(id:id as! NSNumber, displayName: displayName as! String, amount: amount as! NSNumber)
                }

}

func ==(lhs: Item, rhs: Item) -> Bool {
        return lhs.id == rhs.id
}

Solution

The separation into a non-failable and a failable init method is
fine.

The forced casts with as! are safe in your code, because it is verified before
that the three variables id, displayName, number have the
respective types Number, String and NSNumber.

However:

  • The code is error-prone: Changes to the types must be done at two places.



  • It is not immediately obvious that it works correctly.



Forced casts should (and can!) be avoided in most cases, in the
same way as forced unwrappings. The program would be terminated
immediately if the cast fails.

In your code that is quite easy,
by replacing the type checks is T by conditional casts as? T.
The latter return a value of the type T and that makes the forced
cast unnecessary. In addition, it simplifies and shortens the code
of the failable init method:

init?(dictionary: NSDictionary) {

    guard let id = dictionary["id"] as? NSNumber else {
        return nil
    }
    guard let displayName = dictionary["display_name"] as? String else {
        return nil
    }
    guard let amount = dictionary["amount"] as? NSNumber else {
        return nil
    }
    self.init(id: id, displayName: displayName, amount: amount)
}


As a further simplification, combine the optional bindings into
a single guard statement (attribution goes to @Knight0fDragon who
reminded my in a comment):

init?(dictionary: NSDictionary) {
    guard
        let id = dictionary["id"] as? NSNumber,
        let displayName = dictionary["display_name"] as? String,
        let amount = dictionary["amount"] as? NSNumber else {
            return nil
    }
    self.init(id: id, displayName: displayName, amount: amount)
}


Some other points:

  • Your spacing is not consistent.



  • The indenting is totally wrong.



  • The correct spelling is dictionary.

Code Snippets

init?(dictionary: NSDictionary) {

    guard let id = dictionary["id"] as? NSNumber else {
        return nil
    }
    guard let displayName = dictionary["display_name"] as? String else {
        return nil
    }
    guard let amount = dictionary["amount"] as? NSNumber else {
        return nil
    }
    self.init(id: id, displayName: displayName, amount: amount)
}
init?(dictionary: NSDictionary) {
    guard
        let id = dictionary["id"] as? NSNumber,
        let displayName = dictionary["display_name"] as? String,
        let amount = dictionary["amount"] as? NSNumber else {
            return nil
    }
    self.init(id: id, displayName: displayName, amount: amount)
}

Context

StackExchange Code Review Q#138460, answer score: 6

Revisions (0)

No revisions yet.