UPGRADE YOUR SKILLS: Learn advanced Swift and SwiftUI on Hacking with Swift+! >>

Day 25 code challenge - I would love some code reviews and feedback - Thank you

Forums > 100 Days of SwiftUI

I would love for people to review my code and provide any helpful feedback if there are more effective and concise ways of coding this.

Thank you

enum Moves: Int, CaseIterable { // Listed in order of appearance case rock case paper case scissors

var string: String {
    var _string = ""

    switch self {
    case .rock: _string = "Rock"
    case .paper: _string = "Paper"
    case .scissors: _string = "Scissors"
    }

    return _string
}

}

enum Players: Int, CaseIterable { // Listed in order of appearance case player case app case none

var string: String {
    var _string = ""

    switch self {
    case .player: _string = "Player"
    case .app: _string = "App"
    case .none: _string = "None (tie)"
    }

    return _string
}

}

enum Results: Int, CaseIterable { // Listed in order of appearance case win case loose case tie

var string: String {
    var _string = ""

    switch self {
    case .win: _string = "Win"
    case .loose: _string = "Loose"
    case .tie: _string = "Tie"
    }

    return _string
}

}

struct ContentView: View {

// Game
@State private var appsScore = 0
@State private var playersScore = 0
let roundLimit = 10
var moves = ["Rock", "Paper", "Scissors"]
@State private var roundsPlayed = 0
@State private var shouldWin = Bool.random()
@State private var showResults = false

// Round
var gameWinner: Players {
    var _currentRoundWinner: Players = .none

    if playersScore > appsScore {
        _currentRoundWinner = .player
    } else if appsScore > playersScore {
        _currentRoundWinner = .app
    }

    return _currentRoundWinner
}
@State private var appsMove = 0
@State private var appsResultChoice = 0
@State private var playersMove = 0
@State private var roundWinner = 0

var body: some View {
    ZStack {
        LinearGradient(gradient: Gradient(colors: [.black, .gray, .blue]), startPoint: .topLeading, endPoint: .bottom)

        VStack(spacing: 20){
            HStack {
                Image(systemName: "triangle.fill")
                    .foregroundColor(.black)
                Text("Rock")
                    .font(.title)
                    .foregroundColor(.black)
            }
            HStack {
                Image(systemName: "doc.fill")
                    .foregroundColor(.gray)
                Text("Paper")
                    .font(.title)
                    .foregroundColor(.gray)
            }
            HStack {
                Image(systemName: "scissors")
                    .foregroundColor(.blue)
                Text("Scissors")
                    .font(.title)
                    .foregroundColor(.blue)
            }

            Text("Choose a move")
                .font(.title3)
                .foregroundColor(.yellow)

            Picker("MovePicker", selection: $playersMove) {
                ForEach(Moves.allCases, id: \.rawValue) { move in
                    Text("\(move.string)").tag(move)
                }
            }
            .frame(width: 250, height: 30, alignment: /*@START_MENU_TOKEN@*/.center/*@END_MENU_TOKEN@*/)
            .pickerStyle(SegmentedPickerStyle())

            Button(action: {
                if roundsPlayed < roundLimit {
                    self.playRound()
                } else {
                    showResults = true
                }
            }, label: {
                ZStack {
                    Color.black
                        .frame(width: 120, height: 40)
                        .clipShape(Capsule())
                        .overlay(Capsule().stroke(Color.gray, lineWidth: 1))
                        .shadow(color: .gray, radius: 2)
                    Text("\(roundsPlayed < roundLimit ? "Play Round" : "Show Results")")
                        .foregroundColor(.white)
                }
            })

            if roundsPlayed > 0 && roundsPlayed <= roundLimit {
                Text("Round \(roundsPlayed)")
                    .font(.title)
                    .foregroundColor(.black)

                Text("Player's move = \(moves[playersMove])")

                Text("App's move = \(moves[appsMove])")

                Text("App's result choice = \(Results.allCases[appsResultChoice].string)")

                Text("Round winner = \(Players.allCases[roundWinner].string)")
            }
        }
        }
    .alert(isPresented: $showResults, content: {
        Alert(title: Text("Results"), message: Text("Player wins = \(playersScore)\nApp wins = \(appsScore)\nGame winner = \(gameWinner.string)"), dismissButton: .default(Text("OK")) {

            // Post-game reset
            if roundsPlayed == roundLimit {
                roundsPlayed = 0
                playersMove = Moves.rock.rawValue
                appsMove = Moves.scissors.rawValue
                appsResultChoice = Results.win.rawValue

                playersScore = 0
                appsScore = 0
            }
        })
    })
}

//==================================================
// MARK: - Methods
//==================================================

private func assessRoundWinner() {
    if appsResultChoice == Results.win.rawValue &&
        (playersMove == Moves.rock.rawValue && appsMove == Moves.scissors.rawValue ||
            playersMove == Moves.scissors.rawValue && appsMove == Moves.paper.rawValue ||
            playersMove == Moves.paper.rawValue && appsMove == Moves.rock.rawValue) ||
        appsResultChoice == Results.loose.rawValue &&
        (playersMove == Moves.scissors.rawValue && appsMove == Moves.rock.rawValue ||
            playersMove == Moves.paper.rawValue && appsMove == Moves.scissors.rawValue ||
            playersMove == Moves.rock.rawValue && appsMove == Moves.paper.rawValue) ||
        appsResultChoice == Results.tie.rawValue && playersMove == appsMove {

        playersScore += 1
        roundWinner = Players.player.rawValue

    } else if appsResultChoice == Results.win.rawValue &&
        (appsMove == Moves.rock.rawValue && playersMove == Moves.scissors.rawValue ||
            appsMove == Moves.scissors.rawValue && playersMove == Moves.paper.rawValue ||
            appsMove == Moves.paper.rawValue && playersMove == Moves.rock.rawValue ||
            playersMove == appsMove) ||
        appsResultChoice == Results.loose.rawValue &&
        (appsMove == Moves.scissors.rawValue && playersMove == Moves.rock.rawValue ||
            appsMove == Moves.paper.rawValue && playersMove == Moves.scissors.rawValue ||
            appsMove == Moves.rock.rawValue && playersMove == Moves.paper.rawValue ||
            playersMove == appsMove) ||
        appsResultChoice == Results.tie.rawValue && playersMove != appsMove {

        appsScore += 1
        roundWinner = Players.app.rawValue

    } else {
        roundWinner = Players.none.rawValue
    }
}

private func makeAppsMove() {
    appsMove = Int.random(in: 0..<Moves.allCases.count)
    appsResultChoice = Int.random(in: 0..<Results.allCases.count)
    assessRoundWinner()
}

private func playRound() {
    if roundsPlayed < roundLimit {
        makeAppsMove()
    } else {
        showResults = true
    }

    roundsPlayed += 1
}

}

struct ContentView_Previews: PreviewProvider { static var previews: some View { ContentView() } }

2      

The 2 things that jump out are: It's more complicated than it needs to be, and it is missing some things.

The challenge was to have the app choose a random move, then tell the player whether they should win or lose (in your case also maybe tie) and then the player will make their choice accordingly. I can't see where you show these details to the player, and based on the code, it seems to be completely random.

For your 3 types, Moves, Players and Results, these are not all necessary.

Moves adds no benefit here. You can iterate over your array in your picker. Everything else works with just that.

Players should not have a none case. Since there are always 2 players... the app and the user The none case is misleading. you have a Results that covers the tie based on what happens.

Results may provide some benefit, but ultimately, you can just use a string to display the message.

This reduces the complexity for gameWinner since you can make it a String and just use the if statement to return the relevant string. So if playerScore is greater than appScore return "Player"

tip: appsScore (and the other app related variables) should drop the additional s. I understand you mean app's Score, but it is usually much cleaner to just say appScore. Just something to keep in mind... no big deal here.

You also are not making use of the shouldWin boolean, which would help clean up the assessWinner if statements. So it could become:

if shouldWin {
  // make sure player won
} else {
  // make sure player lost

Now part of the challenge was in determining the winning hand in a way that would not require such explicit checks. One approach would be to look at the relationship between them. Rock loses to Paper (moves[0] and moves[0+1]) and Paper (moves[1]) loses to Scissors (moves[1+1])... and Rock wins against Scissors(moves[0+2]) you can then nest these checks within the if shouldWin statements.

Your also assessing the round winner within the makeAppsMove function. But the player hasn't even made their choice yet.

Bonus Tip: You can use a different symbol for rock if you like: circle.grid.hex.fill

You completed a project. That's great. And now you get to refactor it, and work on it to make it better. That's even more fun... but can be frustrating.

If you want to discuss things even further, maybe look at how you could implement your enums in a different way to achieve the end result a little "better" let me know.

Great start... hope you keep at it.

3      

Thank you very much @MarcusKay for not only taking time to review my code but also for giving some feedback. That's very awesome, and appreciated.

I will review your feedback and make a plan to refactor.

Thanks again :)

3      

Hacking with Swift is sponsored by Essential Developer

SPONSORED Join a FREE crash course for mid/senior iOS devs who want to achieve an expert level of technical and practical skills – it’s the fast track to being a complete senior developer! Hurry up because it'll be available only until April 28th.

Click to save your free spot now

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.

 
Unknown user

You are not logged in

Log in or create account
 

Link copied to your pasteboard.