1

Summary:

I made a mistake in my Swift code and I've fixed it. Then I asked myself why this happened and how I could avoid it. I tried some ways but nothing helps.

I put the mistake and my thinking below. I hope you could teach me the right method to avoid this kind of mistake, but any idea, hint, or suggestion would be appreciated.

How can I avoid this kind of logic error?

The following is an excerpt from my assignment for Stanford cs193p course.

class SomeClass {

    ...

    var accumulator: Double?

    func someFunc() {
        // I use `localAccumulator` and write `self.` for clarity.
        if let localAccumulator = self.accumulator { // A
            // `self.accumulator` is modified in this method
            performPendingBinaryOperation() // B
            pendingBinaryOperation = PendingBinaryOperation(firstOperand: localAccumulator) // C
        }
    }

    private func performPendingBinaryOperation() {
        accumulator = pendingBinaryOperation.perform(with: accumulator)
    }

    ...

}

The problem here is that the line B has changed the value of instance value self.accumulator, and the line C should use the new value stored in self.accumulator, but it uses the outdated local var localAccumulator which was copied from self.accumulator's old value.

It was easy to find out the logic error via debugger. But then I reflected on my mistake, and was trying to look for a method to avoid this kind of logic error.

Method 1: use nil checking rather than optional binding

if self.accumulator != nil { // A
    // `self.accumulator` is modified in this method
    performPendingBinaryOperation() // B
    pendingBinaryOperation = PendingBinaryOperation(firstOperand: self.accumulator!) // C
}

Actually, what really matters here is the force unwrapped self.accumulator!, it ensures the value comes from the real source. Using nil checking rather than optional binding can force me to force unwrap on self.accumulator.

But in some Swift style guides(GitHub, RayWenderlich, LinkedIn), force unwrapping is not encouraged. They prefer optional binding.

Method 2: use assertions.

if localAccumulator = self.accumulator { // A
    // `self.accumulator` is modified in this method
    performPendingBinaryOperation() // B
    assert(localAccumulator == self.accumulator) // D
    pendingBinaryOperation = PendingBinaryOperation(firstOperand: localAccumulator) // C
}

I insert a assertion to check whether the localAccumulator is still equal to self.accumulator. This works, it will stop running once self.accumulator is modified unexpectedly. But it's so easy to forget to add this assertion line.

Method 3: SwiftLint

To find a way to detect this kind of error, I've skimmed SwiftLint's all rules, and also got a basic understanding of SourceKitten(one of SwiftLint's dependencies). It seems too complicated to detect this kind of error by SwiftLint, especially when I make this pattern more general.

Some similar cases

Case 1: guard optional binding

func someFunc() {
    guard let localAccumulator = self.accumulator { // A
        return
    }
    // `self.accumulator` is modified in this method
    performPendingBinaryOperation() // B
    pendingBinaryOperation = PendingBinaryOperation(firstOperand: localAccumulator) // C
}

In this case, it's much more difficult for human to notice the error, because localAccumulator has a broader scope with guard optional binding than if optional binding.

Case 2: value copy caused by function passing parameters

// Assume that this function will be called somewhere else with `self.accumulator` as its argument, like `someFunc(self.accumulator)`
func someFunc(_ localAccumulator) {
    // `self.accumulator` is modified in this method
    performPendingBinaryOperation() // B
    pendingBinaryOperation = PendingBinaryOperation(firstOperand: localAccumulator) // C
}

In this case, localAccumulator copies from self.accumulator when this function is called, then self.accumulator changes in the line B, the line C expects the self.accumulator's new value, but get its old value from localAccumulator.

In fact, the basic pattern is as below,

var x = oldValue
let y = x
functionChangingX() // assign x newValue
functionExpectingX(y) // expecting y is newValue, but it's oldValue

x ~ self.accumulator

y ~ localAccumulator

functionChangingX ~ performPendingBinaryOperation

functionExpectingX ~ PendingBinaryOperation.init

This error pattern looks like so common that I guess there should be a name for this error pattern.

Anyway, back to my question, how can I avoid this kind of logic error?

goxcif
  • 11
  • 3

1 Answers1

0

This example shows what I understood from your problem:

var name: String? = "Mike"

func doSomething() {

    // Check if "name" is set (not nil)
    if let personName = name {

        addLastNameToGlobalName() // modifies global var "name"

        // 
        // Here, you want to use the updated "name" value. As you mention, at this point, 
        // "name" could/might/should be different than "personName" (it could
        // even be nil).
        // 
        greet(person: ???) // use "name"? (which is optional), "personName"?, or what?

    }
}

To me, the general approach is the problem here. The fact that you are:

  1. Checking that a global optional-value is not nil
  2. Calling a function that alters this global optional-value
  3. Wanting to use the updated global optional-value as a non-optional value

A simple change in the approach/design of greeting this person would allow you to avoid the type of "logic error" that you mention.

Example 1:

var name: String? = "Mike"

func doSomething() {

    // Check if "name" is set (not nil)
    if let personName = name {
        greet(person: fullName(for: personName))
    }
}

Example 2:

var name: String? = "Mike"

func doSomething() {

    // Check if "name" is set (not nil)
    if let personName = name {
        let fullName = addLastNameToGlobalName() // modifies global var "name" and returns the new value
        greet(person: fullName) 
    }
}
R.B.
  • 422
  • 4
  • 10