2
\$\begingroup\$

Basically What I have is basket ball App. In which I have Added some levels and difficulties. logic is simply use tupple in enum and I have some methods to update / get current level with new values

I have created following code

enum Difficulty:String {
    case easy
    case medium
    case hard
    func getRatioForTargetAgaintsAvailable() -> Float {
        switch  self {
        case .easy:
            return 0.3
        case .medium:
            return 0.5
        case .hard :
            return 0.8
        }
    }

    func getDistanceForTarget () -> Float {
        switch  self {
        case .easy:
            return 5
        case .medium:
            return 6
        case .hard :
            return 7
        }
    }

}

And main enum

enum Level:CustomStringConvertible {
    var description: String {
        return "Welcome to New  \(self.getDifficulty().rawValue) Level , Your Target is \(getTargetValue()), You have \(getAvailableBalls()) Balls left"
    }

    case level1(avaiableBalls:Int,difficulty:Difficulty,totalScore:Int)
    case level2(avaiableBalls:Int,difficulty:Difficulty,totalScore:Int)
    case level3(avaiableBalls:Int,difficulty:Difficulty,totalScore:Int)

    //--------------------------------------------------------------------------------

    //MARK:- Set  Methdos

    //--------------------------------------------------------------------------------

    mutating func updateTotalScore (newValue:Int) {
        switch self {

        case .level1(  let availableBall, let  difficulty,  _):
            self = Level.level1(avaiableBalls: availableBall, difficulty: difficulty, totalScore: newValue)

        case .level2(   let availableBall, let  difficulty,   _):
            self = Level.level2(avaiableBalls: availableBall, difficulty: difficulty, totalScore: newValue)


        case .level3(   let availableBall, let  difficulty,   _):
            self = Level.level3(avaiableBalls: availableBall, difficulty: difficulty, totalScore: newValue)

        }
    }

    mutating func updateAvailableBalls (newValue:Int) {
        switch self {

        case .level1( _, let  difficulty,  let totalScore):
            self = Level.level1(avaiableBalls: newValue, difficulty: difficulty, totalScore: totalScore)

        case .level2(   _ , let  difficulty,   let totalScore):
            self = Level.level2(avaiableBalls: newValue, difficulty: difficulty, totalScore: totalScore)


        case .level3(  _ , let  difficulty,   let totalScore):
            self = Level.level3(avaiableBalls: newValue, difficulty: difficulty, totalScore: totalScore)

        }
    }

    mutating func gotoNextLevel() -> Bool {
        switch  self {
        case .level1:
            self = Level.level2(avaiableBalls: 20, difficulty: .medium, totalScore: 0)
            return true
        case .level2:
            self = Level.level2(avaiableBalls: 20, difficulty: .hard, totalScore: 0)
            return true
        case .level3 :
            self = Level.level3(avaiableBalls: 20, difficulty: .hard, totalScore: 0)
            return false


        }
    }

    mutating func restartLevel () {
        switch  self {
        case .level1:
            self = Level.level1(avaiableBalls: 20, difficulty: .easy, totalScore: 0)
        case .level2:
            self = Level.level2(avaiableBalls: 20, difficulty: .medium, totalScore: 0)
        case .level3 :
            self = Level.level3(avaiableBalls: 20, difficulty: .hard, totalScore: 0)

        }
    }


    //--------------------------------------------------------------------------------

    //MARK:-   Get  Methdos

    //--------------------------------------------------------------------------------




    func getTotalScore () -> Int {
        switch self {
        case .level1(_,_, let totalScore) :
            return totalScore
        case .level2(_,_, let totalScore) :
            return totalScore

        case .level3(_,_ ,let totalScore):
            return totalScore
        }
    }

    func getAvailableBalls () -> Int {
        switch self {
        case .level1(let availableBall,_,_) :
            return availableBall
        case .level2(let availableBall,_,_) :
            return availableBall

        case .level3(let availableBall,_,_) :
            return availableBall
        }
    }

    func getTargetValue () -> Int {
        switch self{
        case .level1(  let _, let  difficulty,   _):
            return  Int(difficulty.getRatioForTargetAgaintsAvailable() * Float(20))

        case .level2(   let _, let  difficulty,   _):
            return  Int(difficulty.getRatioForTargetAgaintsAvailable() * Float(20))


        case .level3(   let _, let  difficulty,   _):
            return  Int(difficulty.getRatioForTargetAgaintsAvailable() * Float(20))

        }
    }

    func getMinDistanceFromHook () -> Float {
        switch self{
        case .level1(  _, let  difficulty,   _):
                return difficulty.getDistanceForTarget()

        case .level2(   _, let  difficulty,   _):
            return difficulty.getDistanceForTarget()


        case .level3(   _, let  difficulty,   _):
            return difficulty.getDistanceForTarget()
        }
    }

    func getDifficulty () -> Difficulty {
        switch self{
        case .level1(  _, let  difficulty,   _):
            return difficulty

        case .level2(   _, let  difficulty,   _):
            return difficulty

        case .level3(   _, let  difficulty,   _):
            return difficulty
        }
    }

    func isLevelPassed () -> Bool {
        return getTotalScore() == getTargetValue()
    }


}

Now In my ViewController I create Object

var currentLevel = Level.level1(avaiableBalls: 20, difficulty: .easy, totalScore: 0)

So my Question is is it good way to implement this functionality any suggested improvement in it?

Thanks for reading this

\$\endgroup\$
3
\$\begingroup\$

Remarks and simplifications for your current code

Swift does not use the “get” prefix for getter methods, so

func getDistanceForTarget() -> Float

is better named

func targetDistance() -> Float

Functions which take no argument, have no side-effects, and are idempotent (i.e. return the same value on each invocation) are often better expressed as a read-only computed property, in your case

var targetDistance: Float

See Swift functions vs computed properties on Software Engineering for a detailed discussion.

The above applies to all of your getXXX methods.


The getTotalScore() function can be simplified to

func getTotalScore() -> Int {
    switch self {
    case .level1(_, _, let totalScore),
         .level2(_, _, let totalScore),
         .level3(_, _, let totalScore):
        return totalScore
    }
}

since all cases bind the same variable. The same applies to the following getter methods.


The let preceding the wildcard pattern in

    case .level1(  let _, let  difficulty,   _):

is not needed, and causes a compiler warning.


The explicit conversion to Float in

return  Int(difficulty.getRatioForTargetAgaintsAvailable() * Float(20))

is not needed since 20 can be both an integer and a floating point literal:

return  Int(difficulty.getRatioForTargetAgaintsAvailable() * 20)

The description method of the CustomStringConvertible protocol should provide

A textual representation of this instance.

You are abusing it for a welcome message.

Suggesting an different data structure

Apparently there is a lot of repetition in your code. Adding another level would require that 9 functions are updated.

If I understand it correctly (I am not a basketball expert!), the Level type is used to store

  • A level (1, 2, 3). Each level is bound to a Difficulty (1 = easy, 2 = medium, 3 = hard).
  • An integer available balls.
  • An integer total score.

If we make Difficulty an Int-based enumeration then we can use it directly as the “level” instead of maintaining that correspondence at several places:

enum Difficulty: Int {
    case easy = 1
    case medium
    case hard
}

Level now becomes a struct with three (independent) properties. This makes many of the getter methods obsolete:

struct PlayerStatus {

    var level: Difficulty
    var availableBalls: Int
    var totalScore: Int
}

(I'm using a different name here to avoid confusion with the level property.)

With an init method

init(level: Difficulty = .easy) {
    self.level = level
    availableBalls = 20
    totalScore = 0
}

the creation of an initial status becomes as easy as

var currentStatus = PlayerStatus()

Because of the integer-based enum Difficulty, updating to the next level can be implemented using the rawValue – without a switch/case and independent of the number of levels:

mutating func gotoNextLevel() {
    if let newLevel = Difficulty(rawValue: self.level.rawValue + 1) {
        self = PlayerStatus(level: newLevel)
    } else {
        // Already at the highest level.
    }
}

Putting it all together, we have

enum Difficulty: Int {
    case easy = 1
    case medium
    case hard

    var targetRatio: Float {
        switch  self {
        case .easy:
            return 0.3
        case .medium:
            return 0.5
        case .hard:
            return 0.8
        }
    }

    var targetDistance: Float {
        switch  self {
        case .easy:
            return 5
        case .medium:
            return 6
        case .hard:
            return 7
        }
    }
}

struct PlayerStatus {

    var level: Difficulty
    var availableBalls: Int
    var totalScore: Int

    init(level: Difficulty = .easy) {
        self.level = level
        availableBalls = 20
        totalScore = 0
    }

    mutating func restartLevel() {
        self = PlayerStatus(level: self.level)
    }

    mutating func gotoNextLevel() {
        if let newLevel = Difficulty(rawValue: self.level.rawValue + 1) {
            self = PlayerStatus(level: newLevel)
        } else {
            // Already at the highest level.
        }
    }

    var targetValue: Int {
        return Int(level.targetRatio * 20)
    }

    var levelPassed: Bool {
        return totalScore == targetValue
    }
}
\$\endgroup\$
  • \$\begingroup\$ Thank you so much :) I really appreciate it (You are awesome :-]) Looking forward to get more tips from you \$\endgroup\$ – Prashant Tukadiya Sep 28 '18 at 4:49
  • \$\begingroup\$ @Martin R Don’t you think that having “get” prefix is much more clearer in the intent and easy to understand, in general? \$\endgroup\$ – Badhan Ganesh Sep 29 '18 at 16:45
  • 4
    \$\begingroup\$ @BadhanGanesh: You'll find the “get” prefix nowhere in the Swift standard library. Such a convention already existed in Objective-C/Cocoa, compare developer.apple.com/library/archive/documentation/Cocoa/…. \$\endgroup\$ – Martin R Sep 29 '18 at 17:34

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.