0
\$\begingroup\$

During an interview I was asked to design the menu for a coffee shop employee so that the employee would go and add different items to the menu. This is the design that I came up with.

I just want to know what other things I'm expected to think of. Are there any problems/complexities that I haven't taken into account?

One other important question is my usage of struct/class. I just want to know if there is anything that I need to be considerate of.

My main objects are:

  • The Product object.
  • The Region object. It contains different stores of different types.
  • The MenuHandler object. It just adds the products to a region or store. It also allows adding a certain snack to a certain product.
  • I've also added a restriction that each product has certain acceptableRegions. So if an employee attempts to add it to a non-acceptable region then an error is to be thrown.

import UIKit
import Foundation

struct Product{
    let id : String
    let name : String
    let price : String
    let imageURL: URL
    var snack: Snack?

    let requirement: Requirement
    var acceptedRegions = Set<Region>()
}

class Region : Hashable {
    let id : String
    let name: String
    var stores : [Store]

    init(id: String, name: String) {
        self.id = id
        self.name = name
    }
    var hashValue: Int {
        return id.hashValue
    }

    static func == (lhs: Region, rhs: Region) -> Bool{
        return lhs.id == rhs.id
    }
}

enum StoreType {
    case normal
    case superStore
}

class Store {
    let id: String
    let type: StoreType
    var products : [Product] = []

    init(id: String, type: StoreType) {
        self.type = type
        self.id = id
    }
}

struct Requirement{
    let neededTime : TimeInterval
    let skillLevel : Employees
    let device : Devices
    let material : [ResourceAllocation]
}

struct Employees : OptionSet{
    let rawValue: Int

    static let junior = Employees(rawValue: 1 << 0)
    static let senior = Employees(rawValue: 1 << 1)
    static let manager = Employees(rawValue: 1 << 2)

    static let all : Employees = [.junior, .senior, .manager]
}

struct Devices : OptionSet{
    let rawValue: Int

    static let expressoMaker = Devices(rawValue: 1 << 0)
    static let coffeeMaker = Devices(rawValue: 1 << 1)
    static let mixer = Devices(rawValue: 1 << 2)
}

enum Resource : String{
    case milk
    case sugar
    case cream
    case coffeeBag
    case teaBag
}

struct ResourceAllocation{
    let resource : Resource
    let amount : Int
}

struct Snack{

}

protocol MenuHandlerType{
    typealias ProductID = String
    typealias RegionID = String
    var products : [ProductID: Product] {get set}
    func add(_ product: Product, to region: Region)
    func add(_ product: Product, to store: Store, in region: Region)
    func add(_ snack: Snack , to product: Product)

}

class MenuHandler: MenuHandlerType {

    var products: [ProductID : Product] = [:]
    var regions : [RegionID: Region]

    required init(regions: [RegionID : Region]) {
        self.regions = regions
    }
    func add(_ product: Product, to region: Region) {
        guard let _region = regions[region.id] else {
            print("You need to first add this region")
            return
        }
        _region.stores.forEach { (store) in
            store.products.append(product)
        }
    }
    func add(_ product: Product, to store: Store, in region: Region) {
        guard let _region = regions[region.id] else {
            print("You need to first add this region")
            return
        }
        if _region.stores.contains(where: { (_store) -> Bool in
            store.id == _store.id
        }){
            print("You need to add this store to this region")
            return
        }

    }

    func add(_ snack: Snack, to product: Product) {
        <#code#>
    }
}

class AddProductsViewController: NSObject{

    let menuHandler : MenuHandler

    init(menuHandler: MenuHandler) {
        self.menuHandler = menuHandler
    }

    // this will be called by a button.
    @objc func add(_ product: Product ,to region: Region){
        if product.acceptedRegions.contains(region){
            menuHandler.add(product, to: region)
        }else {
            print("this product is not FDA approved in this region")
        }

    }
    // add other buttonActions for adding Snack and adding product to store.
}
\$\endgroup\$
  • \$\begingroup\$ why do you do this? _region = regions[region.id]? Why not just use the region they provide you as a param \$\endgroup\$ – Alexander Oct 27 '18 at 3:24
  • \$\begingroup\$ It was more of a design decision. Not sure what I was thinking. More of that a region should exist by itself and if it doesn't then the product shouldn't get added. But doesn't seem to make much sense now \$\endgroup\$ – Honey Oct 27 '18 at 4:07
  • \$\begingroup\$ No, I don't think it does. Either enforce a precondition, or don't. Don't complain about a violated precondition, but then treat it differently anyway. \$\endgroup\$ – Alexander Oct 27 '18 at 6:03
  • \$\begingroup\$ Do you mean the menuHandler should have an addRegion/addStore function. And if I ever attempt to addProducts to a region and my regions are empty then I just throw an error saying, hey you need to add regions/stores first? \$\endgroup\$ – Honey Oct 27 '18 at 13:31
3
\$\begingroup\$

I'll go through this, type by type:

Product

price

I'm not a fan of String price. It's rather see an priceCents: Int or price: Decimal, with a formattedPrice: String computed property, that uses a currency formatter.

snack

What's a product.snack?

requirement

Poorly named IMO. It's not clear what requirement it is. I would suggest something like productionRequirements.

region

What region is it? The region it's produced in? The region it's currently stored in? The region it can be shipped/sold to? Not clear.

Region

I need some more details on what exactly a region models.

StoreType

This is a good, idea to use an enum for a finite set of cases. However, if many of the behaviours of Store vary depending on its type, it's better to make a Store protocol, and Store structs that conform to it, implementing the methods in their correct ways. Perhaps even a Store superclass and NormalStore and SuperStore subclasses.

Something like capableEmployeeTypes would be better.

Store

I don't like that the initial value of products can't be specified in the intitializer. If [] is a sensbile default (and I'm not sure that it is), I would rather see:

var products: [Product]

init(id: String, type: StoreType, initialProducts products: [Product]) {
    self.type = type
    self.id = id
    self.products = products
}

Requirement

What kind of requirement is it? It's not clear. I would suggest something like ProductionRequirements.

skillLevel

Some parts of this code make no sense in isolation:

print(someRequirement.skillLevel) // => [.manager]

Wat?

device

You have a variable called neededTime (rather than time), but also device (rather than neededDevice). Decide either prefix everything with needed, or nothing. If you choose to add it everything, then you should probably stay consistent with the type name Requirement, and say requiredTime, requiredDevices, rather than needed*.

Devices

Shouldn't be an OptionSet. It lets you write funky code like device.type = [.expressoMaker, .mixer]. How can it be both? P.s. It's "espresso", not "expresso"

Employees

And Employees sounds like it would be some sort of Collection of Employee objects. I think it would be better to name it something like EmployeeTypes. One of the issues I see here is that it's not clear if these values are mutually exclusive values or not. The fact that it's an OptionSet suggests that mutually exclusivity isn't required. But then you can get weird things like employee.type = [.junior, .senior]. wat?

ResourceAllocation

Perhaps a better name would be QuantifiedResource? I don't like ResoruceAllocation, because when I see that, I would expect an object that can tell me about what the resource is, how much of it is allocated, and how it is allocated.

style:

This comment applies to all the code: conventional Swift style has a space before every {, and no space before ::

struct ResourceAllocation {
    let resource: Resource
    let amount: Int
}

MenuHandlerType

I see why you put Type in the end, to distinguish the protocol from its concrete implementations. However, if you're going to go down this Hungarian notation route anyway, I would rather you at least be precise, and call it MenuHandlerProtocol.

... More coming soon (i'll be back later)

\$\endgroup\$
  • \$\begingroup\$ Thank you so much. Awesome. Can you also suggest of things related to the system design Had you been interviewing me, where there any complexities that you would've liked me to think of? \$\endgroup\$ – Honey Oct 26 '18 at 18:37
  • \$\begingroup\$ Region is just region. It can be used as which regions we want to add this product to ie the employee would be like let's add Mocha to CA. It's also part of a product's acceptableRegions as in Latte is not acceptable in NY due to FDA regulations :) QuantifiedResource was a really good suggestion btw. I was struggling to find a good name \$\endgroup\$ – Honey Oct 26 '18 at 18:43
  • \$\begingroup\$ to me Devices should be an optionSet. What if you need to blend some fruits to put into the drink? Then you need 2 devices... Also Snack is unimplemented. More there to show the system design :/ \$\endgroup\$ – Honey Oct 26 '18 at 18:45
  • \$\begingroup\$ @Honey You would be better off with a Set<Device>. It seperates out the "deviceness" into its own type Device (perhaps an enum?), and it separates out the "setness" into its own type, Set. It composes better, and can scale to more than 32 or 64 items, unlike an OptionSet. \$\endgroup\$ – Alexander Oct 26 '18 at 18:47
  • \$\begingroup\$ @Honey Hey, I missed that, it's been hectic. \$\endgroup\$ – Alexander Oct 31 '18 at 20:25
1
\$\begingroup\$

Your code doesn't compile. As such it fails any reasonable job interview unless you were writing it without the aid of a compiler.


As for the design:

+->[Product]<>-->[Region]<>-->[Store]<>--+
|                                        |
+----------------------------------------+

The cyclical nature of the design is problematic. This code will leak memory all over the place unless very carefully handled.


struct ResourceAllocation {
    let resource: Resource
    let amount: Int
}

I am not a fan of the above at all... What on earth is "1" milk? Is that a pint, a cup, a teaspoon? What about "1" sugar.


Why the MenuHandlerType? A protocol that has no users is kind of useless, even if there are classes/struct that conform to it.


I see that MenuHandler.add(_:to:) allows users to add a product to a region even if the product isn't accepted in that region. The method expects its user's to abide by the rules.

Instead of the Product having an acceptedRegions, I would rather see maybe the Region have an acceptableRecourses.


Sometimes you put a space before a colon and sometimes you don't. Pick one style and stick with it. The official standard is no space before the colon.


You have asked for comments about your functions.

Region.init(id:name:)

This function returns without initializing all stored properties. That's a compile time error.

MenuHandler.init(regions: [Region])

What is to stop a user from accidentally sending a dictionary where the key (RegionID) and the value's ID don't match? This is very dangerous. I would rather see it written with guarantees that this can't happen:

init(regions: [Region]) {
    guard Set(regions.map { $0.id }).count == regions.count else { fatalError("No regions allowed with duplicate IDs") }
    self.regions = Dictionary.init(grouping: regions, by: { $0.id })
        .mapValues { $0.first! }
}

func MenuHandler.add(_ product: Product, to region: Region)

Given that the region already has to exist inside the MenuHandler before this function can be called, and especially given that you only use the id of the Region object passed in, you don't actually need to send an entire Region object. Just send in the Product and the RegionID.

Also, this function unconditionally adds a product to a valid region. This is where the guard to ensure that the product is allowed in the region should be.

As an aside, RegionID is merely a typealias for a String. A user could easily accidentally pass in a ProductID or a store ID (or a product name for that matter) instead of a RegionID. I would rather see RegionID (and all the other IDs for that matter) be turned into a legitimate type:

struct RegionID: RawRepresentable {
    let rawValue: String 
}

This way, there can be no confusion and sending the wrong ID type will be a compile time error.

func MenuHandler.add(_ product: Product, to store: Store, in region: Region)

Again, only the ids of the region and store are used. If that's all the function needs then that's all that needs to be passed into it. And BTW, you forgot to actually do anything in this function.

func MenuHandler.add(_ snack: Snack, to product: Product)

This function isn't implemented either. I can only assume that, like it's previous functions, it looks up the product by ID and sets the Snack object in it. It doesn't add a snack, because there can be only one, it sets the snack. Maybe it should be called set(_:to:) instead.


I'm also concerned about the lack of access restrictions on any of the properties. When I see class invariants implied by the code, but no way to ensure that those invariants always hold, I get nervous about the code's integrity. At minimum, I would expect to see a private (set) restriction on both of MenuHandler's properties.


Another side note, none of these functions have any real behavior; they just wire up the objects in question. In other words, this code doesn't actually do anything. I would question any requirements document that asks me to write an app that doesn't actually do anything.

\$\endgroup\$
  • \$\begingroup\$ Thanks. The cyclical nature of the design is problematic. This code will leak memory all over the place unless very carefully handled. How would you do it then? \$\endgroup\$ – Honey Nov 2 '18 at 19:41
  • \$\begingroup\$ I don't see any reason for a Product to hold a set of Regions. I would likely break that connection, but it's hard to tell without seeing the actual requirements. \$\endgroup\$ – Daniel T. Nov 2 '18 at 19:42
  • \$\begingroup\$ That's not what I intended. You won't add/mutate acceptedRegions at all. Its regions don't have stores/products. It's just there to indicate that product32 is only FDA approved in California, Arizona. I use it to just do a boolean check. Is that still problematic? \$\endgroup\$ – Honey Nov 2 '18 at 19:45
  • \$\begingroup\$ Yes, because it still holds a strong reference to regions... Also, if "you won't add/mutate acceptedRegions at all" then it shouldn't be mutable. If regions only allow particular products to be sold in them, then Region should have an array of "allowedProducts". That would break the cycle. Another option would be to have Product hold a set of Region IDs instead of Regions. \$\endgroup\$ – Daniel T. Nov 2 '18 at 19:49
  • 1
    \$\begingroup\$ I did say "unless very carefully handled." There is no way you can guarantee that a memory cycle doesn't occur other than manual analysis of the code. Changing acceptedRegions to hold IDs guarantees that they won't happen even a year later when someone else is working with the code and you aren't around to supervise. Even so, I would rather see a more tree like structure than a circle. \$\endgroup\$ – Daniel T. Nov 2 '18 at 23:32

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.