BLACK FRIDAY SALE: Save 50% on all my Swift books and bundles! >>

Day 25 Solution - Constructive Criticism Requested

Forums > 100 Days of SwiftUI
@Icemonster13  HWS+

Hello,

I'm looking for some constructive criticism please. Although I added a little on my own this time for fun, my code always seems more verbose than others. Would love some thoughts on how to make things more efficient or the general idea that I am ok with how things are being done.

struct ContentView: View {

    // MARK: - CONSTANTS
    let choices: [String] = ["🪨", "📄", "✂️", "🦎", "🖖🏽", "🪨", "📄", "✂️", "🦎", "🖖🏽"]
    let winMoves: [String] = ["📄", "✂️", "🖖🏽", "🪨", "🦎", "🖖🏽", "🦎", "🪨", "✂️", "📄"]
    let loseMoves: [String] = ["🦎", "🪨", "📄", "🖖🏽", "✂️", "✂️", "🖖🏽", "🦎", "📄", "🪨"]

    // MARK: - @STATE PROPERTIES
    @State private var appChoice: Int = 0                       // Randomize this in the setCriteria function randomly
    @State private var appShouldWin: Bool = true                // Toggle this in the setCriteria function randomly
    @State private var correctAnswers: [String] = []
    @State private var userChoice: Int = 0
    @State private var attemptNumber: Int = 1
    @State private var score: Int = 0
    @State private var answerEvaluation: String = "Right"
    @State private var gameReset: Bool = true

    // MARK: - ALERT PROPERTIES
    @State private var showingAlert: Bool = false
    @State private var isEndGame: Bool = false

    // MARK: - FUNCTIONS
    func setCriteria() {
        if gameReset {
            appShouldWin = Bool.random()
            attemptNumber = 1
            score = 0
        }
        appShouldWin.toggle()
        appChoice = Int.random(in: 0...4)

        correctAnswers.removeAll()

        if appShouldWin {
            correctAnswers.append(winMoves[appChoice])
            correctAnswers.append(winMoves[appChoice + 5])
        } else {
            correctAnswers.append(loseMoves[appChoice])
            correctAnswers.append(loseMoves[appChoice + 5])
        }
    }

    func evaluateAnswer(choice: Int) {
        if correctAnswers.contains(choices[choice]) {
            score += 1
            answerEvaluation = "Right"
        } else {
            score -= 1
            answerEvaluation = "Wrong"
        }
        attemptNumber += 1
        if attemptNumber > 10 {
            isEndGame = true
            gameReset = true
        } else {
            gameReset = false
            showingAlert = true
        }
    }

    // MARK: - BODY
    var body: some View {
        ZStack {
            Color.blue
                .ignoresSafeArea()

            VStack(spacing: 30) {
                Spacer()

                Text("Rock, Paper, Scissors,\nLizard, Spock")
                    .font(.system(size: 40))
                    .multilineTextAlignment(.center)

                Group {
                    Divider()

                    Text("""
                        Scissors cuts Paper, Paper covers Rock,
                        Rock crushes Lizard, Lizard poisons Spock,
                        Spock smashes Scissors,
                        Scissors decapitate Lizard,
                        Lizard eats Paper, Paper disproves Spock,
                        Spock vaporizes Rock, and as it always has...
                        Rock crushes scissors!
                        """)
                        .font(.system(size: 18))
                        .multilineTextAlignment(.center)

                    Divider()
                }

                Group {
                    HStack {
                        Text("The app chose:")
                        Text(choices[appChoice])
                            .font(.title)
                    }
                    VStack(spacing: 0) {
                        HStack {
                            Text("If the player should")
                            Text(appShouldWin ? "WIN" : "LOSE")
                                .foregroundColor(appShouldWin ? Color.green : Color.red)
                                .fontWeight(.bold)
                            Text("...")
                        }
                        Text("Which is the right choice?")
                    }
                }

                HStack(spacing: 10) {
                    ForEach(0..<5) { number in
                        Button {
                            userChoice = number
                            evaluateAnswer(choice: number)
                        } label: {
                            Text(choices[number])
                                .font(.system(size: 40))
                                .padding(0)
                        }
                        .buttonStyle(.borderedProminent)
                        .shadow(radius: 10)
                    }
                } //: Button HStack
                .alert(answerEvaluation, isPresented: $showingAlert) {
                    Button("Continue", action: setCriteria)
                } message: {
                    Text("You chose \(choices[userChoice])")
                }
                .alert("End Of Game", isPresented: $isEndGame) {
                    Button("Reset", action: setCriteria)
                } message: {
                    Text("You scored \(score) out of 10!")
                }

                Group {
                    Spacer()

                    Text("Current Score: \(score)")
                }
            } //: Outer VStack
            .foregroundColor(.white)
            .font(.system(size: 24))
        } //: ZStack
        .onAppear() {
            setCriteria()
        }
    } //: Body
} //: ContentView

   

@Obelix  

Michael observes:

My code always seems more verbose than others. Would love some thoughts on how to make things more efficient.

Your code is verbose because your logic is verbose! A cursory look at your solution leads me to think that you have a background in procedural languages (VisualBasic, C++, JAVA, etc) and haven't quite made the jump to SwiftUI's declarative style.

The good news is that you're learning! The better news is as you gain experience, you'll start to see some inefficiencies and your code will get better and less verbose. It will become more Swifty.

Well you asked for thoughts, and not a lecture. So here are a few to get you started.

MARK

Ten points to Ravenclaw for using MARK tags in your code!

ContentView

Minus ten points from Ravenclaw for naming your view ContentView. Ugh. Try GameView, or something more descriptive!

Verbose Declarations

You declare your structs variables like this:

    // Not very Swifty. And very verbose. 
    @State private var appChoice: Int        = 0      // Compiler knows zero is an Int
    @State private var appShouldWin: Bool    = true   // Compiler knows true is a Bool
    @State private var userChoice: Int       = 0      // etc

Moving forward, trust the force compiler!

    // Less verbose! More Swifty.
    @State private var appChoice    = 0      // Compiler knows zero is an Int
    @State private var appShouldWin = true   // Compiler knows true is a Bool
    @State private var userChoice   = 0      // etc

Calculated Properties

You'll start to learn the benefits of calculated properties as you progress. This is a great way to consolidate your logic while reducing code. Take this use case for example: When do you want to end a game?

You have this code:

 @State private var gameReset: Bool = true

This is referenced in several places in your code, tied to some value (10). It has to be set and reset. This is verbose, and can lead to logic errors trying to keep these syncronized.

The Swift way is to declare it!

 @State private var gameLength  = 5       // Only go five rounds, then the game ends.
 @State private var gameReset   = false   // Yikes! Now you have to keep this syncronized with gameLength.

Instead of scattering logic throughout your code, consider the benefit of asking Swift to calculate if your player is at the end of a game.

 @State private var gameLength  = 5           // Only go five rounds, then the game ends. Change this in ONE place
  private var gameReset: Bool { attemptNumber > gameLength }  // Game resetting logic in ONE place.

This allows you to simplify your logic in your evaluateAnswer function.

// Nice! Declare what you want to do. Push your game logic to ONE place.
// Resetting logic doesn't belong here.
if gameReset {  // can easily change the gameLength, for testing, or in User Preferences
    isEndGame = true
    // gameReset = true   // you don't need to hand cram your logic here.
} else {
    // gameReset = false // you don't need to hand cram your logic here.
    showingAlert = true
}

Declarative Code

Nota Bene: It is no secret that SwiftUI's declarative style is copied from many other languages. Hush now.

   

@Icemonster13  HWS+

Phenomenal answer. This is exactly what I was looking forward and very much appreciated.

I did move to Swift/SwiftUI from VB and C++ (a little bit of HTML and Java) and am really enjoying this learning experience.

Thank you again for the review. I will look to use this for future lessons/projects.

   

@NigelGee  HWS+

Hi @Icemonster13,

I agree with @Obelix remarks about added Type as this can be inferred by the complier but there will be times when you will have to eg var somePoint: CGFloat = 0 (however if you want to then you can but think you find that most people do not).

Also going on general put

  1. Property
  2. Computed Property (with var body: some View first)
  3. Methods (func)

as this is the View as the main code

With that it a bit big so you could factor it out. I started with the first bit of the View. Made a SwiftUI file called HeaderView and put this in. (See ContentView at end to see it used). Yes still used ContentView as I think of this as the first view in a project if you rename it then you will always have to 

struct HeaderView: View {
    var body: some View {
        VStack(spacing: 30) {
            Text("Rock, Paper, Scissors, Lizard, Spock")
                .font(.largeTitle)
                .multilineTextAlignment(.center)

            Divider()

            Text("""
                    Scissors cuts Paper, Paper covers Rock,
                    Rock crushes Lizard, Lizard poisons Spock,
                    Spock smashes Scissors,
                    Scissors decapitate Lizard,
                    Lizard eats Paper, Paper disproves Spock,
                    Spock vaporizes Rock, and as it always has...
                    Rock crushes scissors!
                    """)
            .multilineTextAlignment(.center)
            .font(.body)

            Divider()
        }
        .padding()
    }
}

NOTE change your Fonts size to use general one as these will size up and down depanding on thier size perference. Next look at the the next section. (you put them in Group in your code) Made a SwiftUI file callled GameView. This is little bit more complicated as has elements that change so we need to pass in those.

struct GameView: View {
    let chosen: String
    let appShouldWin: Bool

    var body: some View {
        VStack {
            HStack {
                Text("The app chose:")
                Text(chosen)
                    .font(.title)
            }
            .padding()

            HStack(spacing: 5) {
                Text("If the player should")
                Text("\(appShouldWin ? "WIN" : "LOSE")…")
                    .foregroundColor(appShouldWin ? Color.green : Color.red)
                    .fontWeight(.bold)
            }

            Text("Which is the right choice?")
        }
    }
}

Next looked at .alert as you have two so with a bit of change around was able to use one with these properties. 

// ALERT PROPERTIES
@Published var showingAlert = false
@Published var alertTitle = ""
@Published var alertMessage = ""
@Published var alertButton = ""
@Published var alertAction: (() -> Void)?

Also look at the the starting properties and your put @State private var appChoice: Int = 0 when you can put

@Published var appChoice = Int.random(in: 0...4)
@Published var appShouldWin = Bool.random()
@Published var correctAnswers = [String]()

(I have used @Published but in the View use @State private, will explain why!). Now look at the methods So change the setCriteria to this (it took me some time to get why you put in array two answer but then worked out you can get there are two solution to each one).

func continueGame() {
    appShouldWin = Bool.random()
    appChoice = Int.random(in: 0...4)

    correctAnswers.removeAll()

    if appShouldWin {
        correctAnswers.append(winMoves[appChoice])
        correctAnswers.append(winMoves[appChoice + 5])
    } else {
        correctAnswers.append(loseMoves[appChoice])
        correctAnswers.append(loseMoves[appChoice + 5])
    }
}

Also added

func resetGame() {
    score = 0
    attemptNumber = 1

    continueGame()
}

and change your evaluateAnswer to this. As you see this is where the alert text comes in and the alertAction will be set.

 func evaluateAnswer(choice: Int) {
    attemptNumber += 1

    if attemptNumber > 10 {
        alertTitle = "End Of Game"
        alertMessage = "Your score is \(score) out of 10!"
        alertButton = "Restart"
        alertAction = resetGame
    } else {
        if correctAnswers.contains(choices[choice]) {
            score += 1
            alertTitle = "Correct"
        } else {
            score -= 1
            alertTitle = "Wrong"
        }

        alertButton = "Continue"
        alertMessage = "You have chosen \(choices[choice])"
        alertAction = continueGame
    }

    showingAlert = true
}

Lastly moved to ViewModel, so created a Swift file called ContentViewModel and put this in 

extension ContentView {
    class ViewModel: ObservableObject {
        let choices = ["🪨", "📄", "✂️", "🦎", "🖖🏽", "🪨", "📄", "✂️", "🦎", "🖖🏽"]
        let winMoves = ["📄", "✂️", "🖖🏽", "🪨", "🦎", "🖖🏽", "🦎", "🪨", "✂️", "📄"]
        let loseMoves = ["🦎", "🪨", "📄", "🖖🏽", "✂️", "✂️", "🖖🏽", "🦎", "📄", "🪨"]

        @Published var appChoice = Int.random(in: 0...4)
        @Published var appShouldWin = Bool.random()
        @Published var correctAnswers = [String]()

        @Published var attemptNumber = 1
        @Published var score = 0

        // ALERT PROPERTIES
        @Published var showingAlert = false
        @Published var alertTitle = ""
        @Published var alertMessage = ""
        @Published var alertButton = ""
        @Published var alertAction: (() -> Void)?

        func evaluateAnswer(choice: Int) {
            attemptNumber += 1

            if attemptNumber > 10 {
                alertTitle = "End Of Game"
                alertMessage = "Your score is \(score) out of 10!"
                alertButton = "Restart"
                alertAction = resetGame
            } else {
                if correctAnswers.contains(choices[choice]) {
                    score += 1
                    alertTitle = "Correct"
                } else {
                    score -= 1
                    alertTitle = "Wrong"
                }

                alertButton = "Continue"
                alertMessage = "You have chosen \(choices[choice])"
                alertAction = continueGame
            }

            showingAlert = true
        }

        func continueGame() {
            appShouldWin = Bool.random()
            appChoice = Int.random(in: 0...4)

            correctAnswers.removeAll()

            if appShouldWin {
                correctAnswers.append(winMoves[appChoice])
                correctAnswers.append(winMoves[appChoice + 5])
            } else {
                correctAnswers.append(loseMoves[appChoice])
                correctAnswers.append(loseMoves[appChoice + 5])
            }
        }

        func resetGame() {
            score = 0
            attemptNumber = 1

            continueGame()
        }
    }
}

So now the ContentView has now gone from over 150 lines to just over 50 lines with only the View

struct ContentView: View {
    @StateObject private var vm = ViewModel()

    var body: some View {
        ZStack {
            Color.blue
                .ignoresSafeArea()

            VStack {
               HeaderView()

                GameView(chosen: vm.choices[vm.appChoice], appShouldWin: vm.appShouldWin)
                    .padding(.bottom, 50)

                HStack(spacing: 10) {
                    ForEach(0..<5) { number in
                        Button {
                            vm.evaluateAnswer(choice: number)
                        } label: {
                            Text(vm.choices[number])
                                .font(.system(size: 40))
                                .padding(0)
                        }
                        .buttonStyle(.borderedProminent)
                        .tint(.cyan)
                        .shadow(radius: 10)
                    }
                }
                .alert(vm.alertTitle, isPresented: $vm.showingAlert) {
                    Button(vm.alertButton, action: vm.alertAction ?? vm.continueGame)
                } message: {
                    Text(vm.alertMessage)
                }

                Group {
                    Spacer()

                    Text("Current Score: \(vm.score)")
                }
            }
            .foregroundColor(.white)
            .font(.title)
            .onAppear(perform: vm.continueGame)
        }
    }
}

PS Added .tint to Button to make it stand out from the background a bit.

   

@Icemonster13  HWS+

Thanks @NigelGee. Excellent information and much appreciated.

Admittedly, some of your answer is a beyond my current skill-set (Day 25) but I fully understand the concepts you are showing and feel good about that.

This will really help me moving forward in the 100 Day course.

Thanks again!

   

@NigelGee  HWS+

Glad you appreciated the answer, when I started I was only going to suggest to refactor the body by making new SwiftUI files, but as I was getting to know your app thought of slightly better way to do the logic eg getting rid of the endGame boolean etc. Sorry got carried away, but it was fun and hope you havee fun in your coding.

   

Save 50% in my Black Friday sale.

SAVE 50% To celebrate Black Friday, all our books and bundles are half price, so you can take your Swift knowledge further without spending big! Get the Swift Power Pack to build your iOS career faster, get the Swift Platform Pack to builds apps for macOS, watchOS, and beyond, or get the Swift Plus Pack to learn advanced design patterns, testing skills, and more.

Save 50% on all our books and bundles!

Sponsor Hacking with Swift and reach the world's largest Swift community!

Archived topic

This topic has been closed due to inactivity, so you can't reply. Please create a new topic if you need to.

All interactions here are governed by our code of conduct.

Swift breaks down barriers between ideas and apps, and I want to break down barriers to learning it. I’m proud to make hundreds of tutorials that are free to access, and I’ll keep doing that whatever happens. But if that’s a mission that resonates with you, please support it by becoming a HWS+ member. It’ll bring you lots of benefits personally, but it also means you’ll directly help people all over the world to learn Swift by freeing me up to share more of my knowledge, passion, and experience for free! Become Hacking with Swift+ member.
Click here to visit the Hacking with Swift store >>
 
Unknown user

You are not logged in

Log in or create account
 

Link copied to your pasteboard.