Skip to main content
Code Review

Return to Answer

added 289 characters in body
Source Link
Martin R
  • 24.2k
  • 2
  • 37
  • 95

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

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 difficultieslevels:

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
 }
}

The description method of the CustomStringConvertible protocol should provide

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 difficulties:

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
 }
}

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

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:

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
 }
}
Source Link
Martin R
  • 24.2k
  • 2
  • 37
  • 95

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 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 difficulties:

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
 }
}
default

AltStyle によって変換されたページ (->オリジナル) /