7

Is there a tidier or better way to write the below nested for...in loops in Swift please? Or is using for...in even the correct way to populate my cards?

for cardNumber in 1...3 {
    for cardSymbolIdentifier in 1...3 {
        for cardColorIdentifier in 1...3 {
            for cardShadingIdentifier in 1...3 {
                let card = Card(cardNumber: cardNumber, cardSymbolIdentifier: cardSymbolIdentifier, cardColorIdentifier: cardColorIdentifier, cardShadingIdentifier: cardShadingIdentifier)
                deckOfCards.append(card)
            }
        }
    }
}

It definitely does the job, but I can't find anything in the documentation about writing multiple nested loops.

Many thanks in advance, Andy

7
  • 5
    This looks like a bad structure for your data at first glance. Commented Apr 8, 2019 at 12:35
  • Just curious: Is this for a Set game ? Commented Apr 8, 2019 at 12:41
  • @MartinR it probably is. But that uses nested for loops too. To avoid them, all possible 4-element arrays using 1,2,3 should be generated (similar but not identical to this) Commented Apr 8, 2019 at 12:51
  • @RakeshaShastri How so please? I'd love to know a better way to do it. Commented Apr 8, 2019 at 12:52
  • 4
    In my opinion, your nested loops work perfect for your needs: They are straight forward, easy to read, easy to maintain, and there is no peformance issue at all to be thought of. Using more sophisticated functional stuff like flatMap etc. typically reduces the code size, but does not automatically improve the readability. Commented Apr 8, 2019 at 13:07

3 Answers 3

3

There is absolutely nothing wrong with your for loops. They are excellent, well-written Swift. The only problem with your code is that it forces deckOfCards to be mutable (var), which may be undesirable. If it is, you could use a map, but I don't consider this particularly better Swift, just slightly different.

let d = (1...3).flatMap { number in
    (1...3).flatMap { symbol in
        (1...3).flatMap { color in
            (1...3).map { shading in
                Card.init(cardNumber: number,
                          cardSymbolIdentifier: symbol,
                          cardColorIdentifier: color,
                          cardShadingIdentifier: shading
                )}}}}

I would probably write it this second way, but only for stylistic reasons. Your for loops are absolutely fine.


Note @user28434's comment below. My original version of this had a major bug (it returned the wrong type). I've been writing Swift since the day it was released. I teach Swift. I teach functional programming in Swift. And I screwed it up when writing it on the fly. I would never have made that mistake with a simple for loop. There's a major lesson in there.

Sign up to request clarification or add additional context in comments.

2 Comments

It would be [[[[Card]]]] instead of flat [Card]. Some flattening required.
Lessons from boneheaded mistakes
1

If you Do that in a single loop, then it become arithmetically complex

for i in 0..<81 {
    deckOfCards.append(
        Card(cardNumber: i / 27, cardSymbolIdentifier: i/9 % 3, 
             cardColorIdentifier: i/3 % 3, cardShadingIdentifier: i % 3)
    )
}

or

let deckOfCards = (0..<81).map {

    Card(cardNumber: $0 / 27, cardSymbolIdentifier: $0/9 % 3, 
         cardColorIdentifier: $0/3 % 3, cardShadingIdentifier: $0 % 3)
}

In both example - indexing start with 0, so your class init function should shift indexing little
or
add +1 in every parameter before/after passing

Comments

0

Your code has no optimisation issues at all according to the need but you can make it a little more elegant or swifty (:p)

let values = [1,2,3]

values.forEach { (cardNumber) in
    values.forEach { (cardSymbolIdentifier) in
        values.forEach { (cardColorIdentifier) in
            values.forEach { (cardShadingIdentifier) in

                let card = Card(cardNumber: cardNumber, cardSymbolIdentifier: cardSymbolIdentifier, cardColorIdentifier: cardColorIdentifier, cardShadingIdentifier: cardShadingIdentifier)
                deckOfCards.append(card)
            }
        }
    }
}   

2 Comments

I disagree that forEach is more Swifty. for...in is a much more powerful, consistent, and flexible construct in Swift. At times, members of the core team have even suggested removing forEach because it has misleading behaviors and is redundant. The main reason for it is to provide symmetry with filter and map (and it can be useful there), but it should not be used to replace simple for...in loops IMO.
Why not go one step further and make it functional, by dropping mutation via .append and replacing .forEach with .flatMap (except in innermost iteration, where .map should be used and where you return your Card directly)?

Start asking to get answers

Find the answer to your question by asking.

Ask question

Explore related questions

See similar questions with these tags.