2
\$\begingroup\$

In my efforts to learn the Swift language, I am trying to mix in a pet project along with my learning of the basic language syntax. In essence, i'm trying to write an iOS keypad application to work with my house alarm, which I have utilizing a device from Alarm Decoder.

The device basically opens up a socket which I have successfully written the code to interact with - I will probably post that for review at some point but right now i'm focusing on decoding the messages that I get back from the device.

Based on my knowledge so far of the Swift language, I have written a function which attempts to decode the message, and in turn populate another variable with various properties. Please can you look over the code and provide feedback if I am even going about this the correct way.

I'm not expecting anyone to re-write the function for me, more just point me in the right direction if i'm off the mark or doing something silly; i'm more than happy to go and research any other way you feel appropriate. As said, i'm learning here...

import Foundation

struct AD2PIKeypadMessage {

    var displayMessage: String = ""
    var isReady: Bool = false
    var isArmedAway: Bool = false
    var isArmedHome: Bool = false
    var isChimeEnabled: Bool = false


} // struct

func formatKeypadMessage(var message: String) -> AD2PIKeypadMessage? {

    // Alarm Decoder keypad message protocol information - http://www.alarmdecoder.com/wiki/index.php/Protocol

    // keypad messages from Alarm Decoder generally look like this:
    // [01000001000---------],0e5,[f707000600e5800c0c020000],"ARMED ***AWAY***** ALL SECURE **"

    // ... or with firmware 2.2a.8+ and config bit 0x0008 set, they can look like this:
    // !KPM:[001000010000--------],010,[f70700060010808c08020000],"ARMED ***STAY** ZONE BYPASSED"

    // First check for blank strings...
    if ( message.isEmpty ) {
        return(nil)
    } // if

    // If the message is prefixed with !KPM, remove it
    if ( message.hasPrefix("!KPM:") ) {
        message = message.substringFromIndex(advance(message.startIndex, 5))
    } // if

    // Split our string into an array using the commas - there should be 4 array elements
    let messageArray = message.componentsSeparatedByString(",")
    if ( 4 == messageArray.count ) {

        // The first array element should be a string of 22 characters
        if ( 22 == count(messageArray[0]) ) {

            // ... it should also start and end with square brackets
            if messageArray[0].hasPrefix("[") && messageArray[0].hasSuffix("]") {

                // At this stage, the message is probably valid so generate a message variable so that
                // we can start populating it...

                var kpiReturnMessage: AD2PIKeypadMessage = AD2PIKeypadMessage()
                kpiReturnMessage.displayMessage = messageArray[3]  // The 4th array element is just display text and could be anything

                // Now lets split the alarm properties string up to an array so its easier for us to use
                let alarmPropertiesArray = Array(messageArray[0])

                // Set the various elements in our message variable - remember the 
                // first array element will be the opening square bracket so we ignore that

                if ( "1" == alarmPropertiesArray[1] ) {
                    kpiReturnMessage.isReady = true
                } // if

                if ( "1" == alarmPropertiesArray[2] ) {
                    kpiReturnMessage.isArmedAway = true
                } // if

                if ( "1" == alarmPropertiesArray[3] ) {
                    kpiReturnMessage.isArmedHome = true
                } // if

                if ( "1" == alarmPropertiesArray[9] ) {
                    kpiReturnMessage.isChimeEnabled = true
                } // if

                return(kpiReturnMessage)

            } else {
                return(nil)
            } // if

        } else {
            return(nil)
        } // if

    } else {
        return(nil)
    } // if

} // func




if let kpm: AD2PIKeypadMessage? = formatKeypadMessage("[01000001000---------],0e5,[f707000600e5800c0c020000],\"ARMED ***AWAY***** ALL SECURE **\"") {

    println("Valid Keypad Message")

} // if
\$\endgroup\$
1
\$\begingroup\$

One thing I'm noticing is that you have a lot of "magic values". Things like strings and numbers that need to be explained by a comment. I'd create the following constants:

// Some useful string constants we'll need
let kMessagePrefix = "!KPM:"
let kTrue = "1"
// Important numerical constants
let kValidMessageCount = 4
let kValidMessageLength = 22
let kIsReadyIndex = 1
let kIsArmedAwayIndex = 2
let kIsArmedHomeIndex = 3
let kIsChimeEnabledIndex = 9

You can then do things like this:

if (message.hasPrefix(kMessagePrefix)) {

This way the code describes what you're looking for. It's not just some random string. It's the prefix of all messages.

Next, since you have the prefix as a string, use that string to get the rest of the message like this:

    message = message.substringFromIndex(advance(message.startIndex, count(kMessagePrefix)))

You can remove all of the else cases. In the case where the message is valid and you can get the data out of it, you return a valid record. In all other cases, it falls through. So you can reformat your code as:

if (kValidMessageCount == messageArray.count ) {
    // ... check all the other stuff
    return (kpiReturnMessage)
}

return (nil)

It makes it much easier to read.

Also, you use messageArray[0] a lot. Why not assign it to a variable (or better a constant since you aren't changing it)?

let keypadMessage = messageArray[0];
if (kValidMessageLength == count(keypadMessage)) {
        // ... it should also start and end with square brackets
        if keypadMessage.hasPrefix("[") && keypadMessage.hasSuffix("]") {

            // At this stage, the message is probably valid so generate a message variable so that
            // we can start populating it...

            var kpiReturnMessage: AD2PIKeypadMessage = AD2PIKeypadMessage()
            kpiReturnMessage.displayMessage = messageArray[3]  // The 4th array element is just display text and could be anything

            // Now lets split the alarm properties string up to an array so its easier for us to use
            let alarmPropertiesArray = Array(keypadMessage)

            // Set the various elements in our message variable - remember the 
            // first array element will be the opening square bracket so we ignore that

            if ( kTrue == alarmPropertiesArray[kIsReadyIndex] ) {
                kpiReturnMessage.isReady = true
            } // if

            if ( kTrue == alarmPropertiesArray[kIsArmedAwayIndex] ) {
                kpiReturnMessage.isArmedAway = true
            } // if

            if ( kTrue == alarmPropertiesArray[kIsArmedHomeIndex] ) {
                kpiReturnMessage.isArmedHome = true
            } // if

            if ( kTrue == alarmPropertiesArray[kIsChimeEnabledIndex] ) {
                kpiReturnMessage.isChimeEnabled = true
            } // if

            return(kpiReturnMessage)
    }
}

If you think you're going to check more boolean properties, then you might want to make the if statements into their own function.

\$\endgroup\$
  • \$\begingroup\$ Thank you for this input - what you are saying makes a lot of sense. I will incorporate your suggestions into my code for sure. Thanks again, this was helpful... \$\endgroup\$ – Jon Jun 4 '15 at 14:53

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.