3
\$\begingroup\$

I know there is some repeated code in here, which is why I need a code review. The point of this app is simple, the user opens it, taps "begin", they select their league - men's, women's or co-ed, and then they select their skill level.

The main issue I see with this is the fact that the button functions in the league VCs are all the same. I tried adding it to the BorderedButton.swift file, but it required that I add the player variable - which I figured was redundant to the fact that it is already in each league view controller.

Main View Controller:

class WelcomeVC: UIViewController {

    override func viewDidLoad() {
        super.viewDidLoad()


    }

    @IBAction func unWindFromSkillVC(unwindSegue: UIStoryboardSegue) {

    }

}

User arrives here after they select "continue" on the welcome screen. They then select which league they want to join. This gets the segues ready for the next view controller as well
League View Controller:

import UIKit

class LeagueVC: UIViewController {


    var player: Player!

    override func viewDidLoad() {
        super.viewDidLoad()

        player = Player()
    }


    @IBAction func onMensTapped(_ sender: Any) {
        selectLeague(leagueType: "mens")
        performSegue(withIdentifier: "mensVCSegue", sender: self)

    }

    @IBAction func onWomensTapped(_ sender: Any) {
        selectLeague(leagueType: "womens")
        performSegue(withIdentifier: "womensVCSegue", sender: self)
    }

    @IBAction func onCoedTapped(_ sender: Any) {
        selectLeague(leagueType: "coed")
        performSegue(withIdentifier: "coedVCSegue", sender: self)
    }


    func selectLeague(leagueType: String) {
        player.desiredLeague = leagueType

    }

    override func prepare(for segue: UIStoryboardSegue, sender: Any?) {
        if let mensVC = segue.destination as? MensVC {
            mensVC.player = player
        } else if let womensVC = segue.destination as? WomensVC {
            womensVC.player = player
        } else if let coedVC = segue.destination as? CoedVC {
            coedVC.player = player
        }
    }
}

User arrives here after they select the "co-ed" league, then they select their skill level.
Co-ed View Controller (controls the co-ed view controller):

class CoedVC: UIViewController {

    var player: Player!


    override func viewDidLoad() {
        super.viewDidLoad()

        print(player.desiredLeague)
    }

    override func didReceiveMemoryWarning() {
        super.didReceiveMemoryWarning()
        // Dispose of any resources that can be recreated.
    }


    @IBAction func beginnerBtnPressed(_ sender: Any) {
        selectSkillLevel(skillLevel: "beginner")
    }

    @IBAction func ballerBtnPressed(_ sender: Any) {
       selectSkillLevel(skillLevel: "baller")
    }


    func selectSkillLevel(skillLevel: String) {
        player.selectedSkillLevel = skillLevel
        print(skillLevel)
    }
}

User arrives here after they select the "women's" league, then they select their skill level.
Women's View Controller:

class WomensVC: UIViewController {

    var player: Player!

    override func viewDidLoad() {
        super.viewDidLoad()

        print(player.desiredLeague)
    }

    override func didReceiveMemoryWarning() {
        super.didReceiveMemoryWarning()
        // Dispose of any resources that can be recreated.
    }


    @IBAction func beginnerBtnPressed(_ sender: Any) {
        selectSkillLevel(skillLevel: "beginner")
    }

    @IBAction func ballerBtnPressed(_ sender: Any) {
        selectSkillLevel(skillLevel: "baller")
    }


    func selectSkillLevel(skillLevel: String) {
        player.selectedSkillLevel = skillLevel
        print(skillLevel)
    }

}

User arrives here after they select the "men's" league, then they select their skill level.
Men's View Controller:

import UIKit

class MensVC: UIViewController {

    var player: Player!

    override func viewDidLoad() {
        super.viewDidLoad()

        print(player.desiredLeague)
    }

    override func didReceiveMemoryWarning() {
        super.didReceiveMemoryWarning()
        // Dispose of any resources that can be recreated.
    }


    @IBAction func beginnerBtnPressed(_ sender: Any) {
        selectSkillLevel(skillLevel: "beginner")
    }

    @IBAction func ballerBtnPressed(_ sender: Any) {
        selectSkillLevel(skillLevel: "baller")
    }

    func selectSkillLevel(skillLevel: String) {
        player.selectedSkillLevel = skillLevel
        print(skillLevel)
    }

}

Here is the GitHub link: https://github.com/andrewlundy/app-swoosh

\$\endgroup\$
4
\$\begingroup\$

The first thing that I noticed is that you have three view controller classes (MensVC, WomensVC, CoedVC) with identical code.

You can reduce that to a single view controller class (perhaps SkillsVC ?) which is the custom class for all three view controllers.

The prepare(for:segue:) method in LeagueVC then simplifies as well:

override func prepare(for segue: UIStoryboardSegue, sender: Any?) {
    let skillsVC = segue.destination as! SkillsVC
    skillsVC.player = player
}

This is also a valid use-case for a forced cast: If it fails then we have a programming error, which should be detected early.

You could even replace the three view controllers by a single one in the storyboard. The only difference between them is the background image, and that can be loaded (e.g. in viewDidLoad) depending on the chosen league.

With respect to the duplicated button action code: I find it acceptable for three buttons. An alternative is to add outlet connections to each button in the LeagueVC and connect all buttons to the same action method:

@IBAction func leagueButtonTapped(_ sender: UIButton) {
    switch sender {
    case mensButton:
        selectLeague(leagueType: "mens")
        performSegue(withIdentifier: "mensVCSegue", sender: self)
    case womensButton:
        selectLeague(leagueType: "womens")
        performSegue(withIdentifier: "womensVCSegue", sender: self)
    case coedButton:
        selectLeague(leagueType: "coed")
        performSegue(withIdentifier: "coedVCSegue", sender: self)
    default:
        fatalError()
    }
}

Another option would be to attach the segues directly to the buttons and remove the button action methods. Then the logic has to be put into prepare(for:segue:), depending on the segue's identifier.

\$\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.