1
\$\begingroup\$

I'm using spritekit, using on touch to detect what the users has tapped, ie., no collisions, etc.

I have an array of sprite nodes which in effect is a multi-state boolean switch for now the user can only select one node to be active at a time so the logic looks like:

Node A,  Node B, Node C .........Node M
off       off      off        initial
on        off      off        user touches node A
off       off      on          user touches node C

This code is my working template and would like to reduce a lot of this boilerplate down to something reasonable.

I'm looking for ways that are either faster, more the "Apple Way", more OOP.

    var temp1:SKSpriteNode = SKSpriteNode()
  override func touchesBegan(_ touches: Set<UITouch>, with event: UIEvent?)
  {
    if let touch = touches.first
    {
      let tappedNodes:[sk_Node] = nodes(at: touch.location(in: self) )

      for node in tappedNodes
      {
    switch node.name!
        {
        case "level_1":
          pushd(node: node)
          node.run( sk_Action.animate(with: [sk_Texture(imageNamed: "level_1_selected") ], timePerFrame: K1_4.f64_t) )

          temp1 = childNode(withName: "level_2")!
          temp1.run(sk_Action.animate(with: [sk_Texture(imageNamed: "level_2_normal") ], timePerFrame: K1_4.f64_t) )
          popd(node:temp1)

          temp1 = SKSpriteNode()
          temp1 = childNode(withName: "level_3")!
          temp1.run( sk_Action.animate(with: [sk_Texture(imageNamed: "level_3_normal") ], timePerFrame: K1_4.f64_t) )
          popd(node:temp1)

    case "level_2":
      pushd(node: node)
      node.run( sk_Action.animate(with: [sk_Texture(imageNamed: "level_2_selected") ], timePerFrame: K1_4.f64_t) )

      temp1 = childNode(withName: "level_1")!
      temp1.run( sk_Action.animate(with: [sk_Texture(imageNamed: "level_1_normal") ], timePerFrame: K1_4.f64_t) )
      popd(node:temp1)

      temp1 = SKSpriteNode()
      temp1 = childNode(withName: "level_3")!
      temp1.run( sk_Action.animate(with: [sk_Texture(imageNamed: "level_3_normal") ], timePerFrame: K1_4.f64_t) )
      popd(node:temp1)

    case "level_3":
      pushd(node: node)
      node.run( sk_Action.animate(with: [sk_Texture(imageNamed: "level_3_selected") ], timePerFrame: K1_4.f64_t) )

      temp1 = childNode(withName: "level_2")!
      temp1.run( sk_Action.animate(with: [sk_Texture(imageNamed: "level_2_normal") ], timePerFrame: K1_4.f64_t) )
      popd(node:temp1)

      temp1 = SKSpriteNode()
      temp1 = childNode(withName: "level_1")!
      temp1.run( sk_Action.animate(with: [sk_Texture(imageNamed: "level_1_normal") ], timePerFrame: K1_4.f64_t) )
      popd(node:temp1)

} } } }

pushd and popd are SKActions that do some simple scaling up and down.

\$\endgroup\$
  • \$\begingroup\$ What are sk_Action, sk_Node, etc? Did you define custom types replacing SKAction, SKNode, ...? \$\endgroup\$ – Martin R Nov 14 '17 at 13:52
  • \$\begingroup\$ Yes u typealiased them so its easier to compile for OSX. Sk_Sprite ud SKSpritenode \$\endgroup\$ – μολὼν.λαβέ Nov 14 '17 at 16:36
1
\$\begingroup\$

I am not sure why you need to define custom sk_Node, sk_Action, ... types (or type aliases). SKNode, SKAction, etc are available on all Apple platforms (macOS, iOS, tvOS, watchOS).

But if you do so then the names should conform to the Swift naming conventions:

Names of types and protocols are UpperCamelCase. Everything else is lowerCamelCase.


The type annotation in

var temp1:SKSpriteNode = SKSpriteNode()

is not needed, the compiler can infer the type automatically from the expression on the right-hand side:

var temp1 = SKSpriteNode()

But why do you create and assign a node at all if that is overwritten in

temp1 = childNode(withName: "level_2")!
// do something with `temp1` ...

? And the same happens here:

temp1 = SKSpriteNode()
temp1 = childNode(withName: "level_3")!
// do something with `temp1` ...

So you could just define a variable without assigning a value:

var temp1: SKSpriteNode

// ...
temp1 = childNode(withName: "level_2")!
// do something with `temp1` ...

temp1 = childNode(withName: "level_3")!
// do something with `temp1` ...

But why use a common variable at all? Are you hoping to save some variable storage? Better prefer clarity and use separate variables:

let node2 = childNode(withName: "level_2")!
// do something with `node2` ...

let node3 = childNode(withName: "level_3")!
// do something with `node3` ...

(And the chances are good that the compiler optimizes the variable storage anyway.)

And if there is any chance that a node with the given name does not exist then use optional binding:

if let node2 = childNode(withName: "level_2") {
    // do something with `node2` ...
}

In func touchesBegan you expect that the set of touches is not empty. You can early return in the "exceptional case", which also saves you one level of indentation:

override func touchesBegan(_ touches: Set<UITouch>, with event: UIEvent?) {

    guard let touch = touches.first else { return }
    // ...

}

This is my major point: Handling the different nodes in a switch/case statement does not scale well. If another node is added for another state then in in addition to another case in the switch statement, code has to be added to all other cases as well.

That is tedious and error-prone (and you mentioned nodes "A" ... "M" in your question, i.e. 13 different states).

My suggestion is to create an array switchNodes with all nodes. Then you only have to traverse through the array once and act accordingly:

let tappedNodes = nodes(at: touch.location(in: self))
for tappedNode in tappedNodes {
    for (i, switchNode) in switchNodes.enumerated() {
        if switchNode == node {
            pushd(node: tappedNode)
            let imageName = "level_\(i+1)_selected"
            switchNode.run(SKAction.animate(with: [SKTexture(imageNamed: imageName)], timePerFrame: 1.0) )
        } else {
            let imageName = "level_\(i+1)_normal"
            node.run(SKAction.animate(with: [SKTexture(imageNamed: imageName)], timePerFrame: 1.0) )
            switchNode(node: switchNode)
        }
    }
}

Now states can be added or removed without problems.

\$\endgroup\$
  • \$\begingroup\$ Great! Thanks for the answer and comments. I also have some thoughts to cross compile to android and it's easier to grep/sed though source code with a custom Typealias. The temp node because the textures can be very large and am concerned with memory leaks, flushing the cache which iOS doesn't seem to do frequently. I had considered some kind of struct/protocol in the loop but couldn't work out the details. \$\endgroup\$ – μολὼν.λαβέ Nov 15 '17 at 21:20
1
\$\begingroup\$

If it were me, I'd reduce the boiler plate by only putting what's actually changing into the switch statement, and putting everything else outside of it. So here you would only set the texture names in the switch statement. Something like this:

var selectedTextureName : String
var normalTexture1Name : String
var normalTexture2Name : String
var node1Name : String
var node2Name : String
switch node.name!
{
    case "level_1":
        selectedTextureName = "level_1_selected"
        normalTexture1Name = "level_2_normal"
        normalTexture2Name = "level_3_normal"
        node1Name = "level_2"
        node2Name = "level_3"

    case "level_2":
        selectedTextureName = "level_2_selected"
        normalTexture1Name = "level_1_normal"
        normalTexture2Name = "level_3_normal"
        node1Name = "level_1"
        node2Name = "level_3"

    case "level_3":
        selectedTextureName = "level_3_selected"
        normalTexture1Name = "level_1_normal"
        normalTexture2Name = "level_2_normal"
        node1Name = "level_1"
        node2Name = "level_2"
}

pushd(node: node)
node.run( sk_Action.animate(with: [skTexture(imageNamed:selectedTextureName)], timePerFrame: K1_4.f64_t) )

temp1 = childNode(withName: node1Name)!
temp1.run( sk_Action.animate(with: [sk_Texture(imageNamed: normalTexture1Name) ], timePerFrame: K1_4.f64_t) )
popd(node:temp1)

temp1 = SKSpriteNode()
temp1 = childNode(withName: node2Name)!
temp1.run( sk_Action.animate(with: [sk_Texture(imageNamed: normalTexture2Name) ], timePerFrame: K1_4.f64_t) )
popd(node:temp1)
\$\endgroup\$

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.