Problem
I’m solving the following problem using the Chain Of Responsibility design pattern in Swift:
Not all mechanics are created equally. Some mechanics are more
experienced and can do more than others. We need a system where every
job is propagated from the least experienced mechanic to the most.
This way experienced mechanics that can perform more jobs are not
busy with jobs that more junior mechanics can take care of.
I would love some feedback on how I can improve this. If it remains true to the definition of Chain Of Responsibility or if there are any swift standard coding convention that I should be following.
Here is the code, the full repo can be found here: Design Patterns in Swift: Chain of Responsibility
import Foundation
enum Skill: Int{
case OilChangeOnly = 1, Junior, Apprentice, MasterMechanic
}
class Job{
let minimumSkillSet: Skill
let name: String
var completed: Bool = false
init(minimumSkillSet: Skill, name: String){
self.minimumSkillSet = minimumSkillSet
self.name = name
}
}
class Mechanic{
let skill: Skill
var name: String
var isBusy: Bool = false
init(skill: Skill, name: String){
self.skill = skill
self.name = name
}
func performJob(job: Job) -> Bool{
if job.minimumSkillSet > self.skill || isBusy{
assert(false, "This mechanic is either busy or insufficiently skilled for this job, he should have never been asked to perform it, there is something wrong in the chain of responsibility");
}else
{
isBusy = true
print("(name) with skill set (skill) has started to do (job.name)")
job.completed = true
return true
}
}
}
class MechanicSkillGroup{
var mechanics: [Mechanic]
var nextLevel: MechanicSkillGroup?
var skill: Skill
init(skill: Skill, mechanics: [Mechanic], nextLevel: MechanicSkillGroup?){
self.mechanics = mechanics
self.skill = skill
self.nextLevel = nextLevel
}
func performJobOrPassItUp(job: Job) -> Bool{
if (job.minimumSkillSet > skill || mechanics.filter({$0.isBusy == false}).count == 0){
if let nextLevel = nextLevel{
return nextLevel.performJobOrPassItUp(job)
}else{
print("No one is available to do this job")
return false
}
}else{
if let firstAvailableMechanic = mechanics.filter({$0.isBusy == false}).first{
return firstAvailableMechanic.performJob(job)
}
assert(false, "This should never be reached since our if-else statement is fully exhaustive. You cannot have both all mechanics busy and an available mechanic within one skill group");
}
}
}
class Shop{
private var firstMechanics: MechanicSkillGroup
init(firstMechanics: MechanicSkillGroup){
self.firstMechanics = firstMechanics
}
func performJob(job: Job) -> Bool{
return firstMechanics.performJobOrPassItUp(job)
}
}
Here is main with setup and some test cases
import Foundation
var steve = Mechanic(skill: .MasterMechanic, name: "Steve Frank")
var joe = Mechanic(skill: .MasterMechanic, name: "Joe Alison")
var jack = Mechanic(skill: .MasterMechanic, name: "Jack Ryan")
var brian = Mechanic(skill: .MasterMechanic, name: "Drake Jin")
var masterMechanics = MechanicSkillGroup(skill: .MasterMechanic, mechanics: [steve, joe, jack, brian], nextLevel: nil)
var tyson = Mechanic(skill: .Apprentice, name: "Tyson Trump")
var tina = Mechanic(skill: .Apprentice, name: "Tina Bernard")
var bryan = Mechanic(skill: .Apprentice, name: "Bryan Tram")
var lin = Mechanic(skill: .Apprentice, name: "Lin Young")
var apprenticeMechanics = MechanicSkillGroup(skill: .Apprentice, mechanics: [tyson, tina, bryan, lin], nextLevel: masterMechanics)
var ken = Mechanic(skill: .Junior, name: "Ken Hudson")
var matt = Mechanic(skill: .Junior, name: "Matt Lowes")
var sandeep = Mechanic(skill: .Junior, name: "Sandeep Shenoy")
var tom = Mechanic(skill: .Junior, name: "Tom Berry")
var juniorMechanics = MechanicSkillGroup(skill: .Junior, mechanics: [ken, matt, sandeep, tom], nextLevel: apprenticeMechanics)
var grant = Mechanic(skill: .OilChangeOnly, name: "Grant Hughes")
var larry = Mechanic(skill: .OilChangeOnly, name: "Larry White")
var bryant = Mechanic(skill: .OilChangeOnly, name: "Bryant Newman")
var reza = Mechanic(skill: .OilChangeOnly, name: "Reza Shirazian")
var laura = Mechanic(skill: .OilChangeOnly, name: "Laura Lee")
var arnold = Mechanic(skill: .OilChangeOnly, name: "Arnold Shummer")
var oilChangeOnlyes = MechanicSkillGroup(skill: .OilChangeOnly, mechanics: [grant], nextLevel: juniorMechanics)
var shop = Shop(firstMechanics: oilChangeOnlyes)
var jobs = [Job(minimumSkillSet: .Junior, name: "Windshield Viper"),
Job(minimumSkillSet: .Apprentice, name: "Light Bulb Change"),
Job(minimumSkillSet: .Apprentice, name: "Battery Replacement"),
Job(minimumSkillSet: .OilChangeOnly, name: "General Oil Change"),
Job(minimumSkillSet: .OilChangeOnly, name: "General Oil Change"),
Job(minimumSkillSet: .OilChangeOnly, name: "General Oil Change"),
Job(minimumSkillSet: .OilChangeOnly, name: "General Oil Change"),
Job(minimumSkillSet: .MasterMechanic, name: "Timing Belt Replacement"),
Job(minimumSkillSet: .Junior, name: "Brake Pads Replacement")
]
for job in jobs{
shop.performJob(job)
}
Here is output you’d get with this setup:
Ken Hudson with skill set Junior has started to do Windshield Wiper
Tyson Trump with skill set Apprentice has started to do Light Bulb Change
Tina Bernard with skill set Apprentice has started to do Battery Replacement
Grant Hughes with skill set OilChangeOnly has started to do General Oil Change
Matt Lowes with skill set Junior has started to do General Oil Change
Sandeep Shenoy with skill set Junior has started to do General Oil Change
Tom Berry with skill set Junior has started to do General Oil Change
Steve Frank with skill set MasterMechanic has started to do Timing Belt Replacement
Bryan Tram with skill set Apprentice has started to do Brake Pads Replacement
Program ended with exit code: 9
Solution
Formatting
There are several formatting issues with your code.
The standard is 4 spaces per indentation level. It looks like you’ve opted for 2, but it appears you’ve at least applied this consistently, although I really heavily prefer 4.
Your opening braces frequently appear squished right up against the preceding word. There should be a space.
In at least one place, you’ve left this:
}else
{
Which not only looks bad, but is inconsistent with other parts of your code.
Personally, I prefer not to “cuddle” my else (and catch) keywords, and as such, I write them like this:
}
else {
The primary inspiration behind this brace style has a lot to do with how Xcode handles collapsing code.
You also have a wild semicolon ; hanging out at the end of one of your assert statements.
Most of these things can be addressed automatically with SwiftLint.
assert & print
Neither of these are really particularly helpful.
First of all, if we’re going to log, we should use a real logging library. That’s a really minor issue here. The real problem here is that we’re printing to communicate things that should probably be communicate in the return values (or already are).
As for the asserts, again, I think we’re using these to communicate information that should probably be handled with return values. Or in some cases, we can probably reorganize our logic to simply get rid of them altogether.
Method Naming
What your methods do and how they’re named aren’t exactly in sync.
A method called performJob probably shouldn’t return a Bool indicating whether or not the job could be performed. And methods should generally never have Or or And as part of their name (there are exceptions, but they are rare).
The performJobOrPassItUp method probably makes a lot more sense framed in this way:
func firstAvailableMechanicForJobWithSkillLevel(skillLevel: Skill) -> Mechanic?
That is to say… the method either returns a mechanic or returns nil. Returning nil indicates there are no available mechanics and whatever is looking to get a job of that skill level accomplished must look elsewhere or try again later. Otherwise, it passes back out a mechanic which can do the job.
Boolean literals and checking if arrays are empty
mechanics.filter({$0.isBusy == false}).count == 0
First of all, we should almost never be comparing against boolean literals. We can simply write !$0.isBusy.
Second of all, if we want to know whether an array is empty, we should check isEmpty:
mechanics.filter({!$0.isBusy}).isEmpty
Initializers
In both your Mechanic and Job classes, you don’t have a way to initialize all of the variables. In both cases, you default initialize a Bool value. Why not just make that part of the initializers, but give it a default value.
class Job{
let minimumSkillSet: Skill
let name: String
var completed: Bool
init(minimumSkillSet: Skill, name: String, completed: Bool = false) {
self.minimumSkillSet = minimumSkillSet
self.name = name
self.completed = false
}
}
String-Typing
The Mechanic class has a name property which is a string. And this is fine. The name here is nothing more than an identifier which we might want to use to display to the user or something.
But the Job class’s name property is doing something completely different. We may still want a pretty string to display to the user, but this property is doing more than that. I think we need a full fledged type here. Why, for example, would a "General Oil Change" ever require anything other than the OilChangeOnly skill? So the list of jobs is surely countable, and the actual skill level for any particular job is surely constant.