1
\$\begingroup\$

I'm making a photo marker application and need to make a factory pattern for marker. I think it is not very flexible and overall not good.

Would you check my code and suggest what could be improved?

import Foundation


enum MarkerType: String {
    case Shape, Image
}

enum MarkerError: ErrorType {
    case ImageNoExist
    case ShapeNoExist
}

struct ImageMarker {
    static func make(type: String) -> UIImageView {
        var imageStr: String = ""
        switch type {
        case "X":
            imageStr = "close.png"
            break
        default: break
        }

        let image = UIImage(named: imageStr)
        let tintedImage = image?.imageWithRenderingMode(.AlwaysTemplate)
        let imageView = UIImageView(image: tintedImage)
        imageView.tintColor = UIColor.redColor()
        return imageView
    }
}

struct ShapeMarker {
    static func make(type: String) -> UIView {
        var shape: UIView = UIView()
        switch type {
        case "CIRCLE":
            //shape = CircleView()
            break
        default: break
        }

        return shape
    }
}

typealias Factory = (String) -> AnyObject

class MarkerHelper {
    class func factoryFor(type: MarkerType) -> Factory {
        switch type {
        case .Shape:
            return ShapeMarker.make
        case .Image:
            return ImageMarker.make
        }
    }
}
\$\endgroup\$

closed as unclear what you're asking by nhgrif, mdfst13, SuperBiasedMan, Marc-Andre, Ethan Bierlein Apr 4 '16 at 16:09

Please clarify your specific problem or add additional details to highlight exactly what you need. As it's currently written, it’s hard to tell exactly what you're asking. See the How to Ask page for help clarifying this question. If this question can be reworded to fit the rules in the help center, please edit the question.

  • \$\begingroup\$ I don't understand what you're really trying to do here. Can you elaborate the plain-English part of the question? Maybe explain why you think you need to implement a factory pattern? It's something we just really don't see in iOS/Objective-C/Swift. \$\endgroup\$ – nhgrif Apr 3 '16 at 17:44
  • 1
    \$\begingroup\$ I think what is needed here is an example of use. Can you show code that uses the above constructs? \$\endgroup\$ – Daniel T. Apr 3 '16 at 22:45
  • \$\begingroup\$ @nhgrif for example, i am making photo marker app. there is a imagebox and you can make a marker on image box. A marker has two type, image marker and shape(like circle created by core graphic). and i am going to make a factory of markers generate both. \$\endgroup\$ – kangtaku Apr 5 '16 at 2:13
  • \$\begingroup\$ I could assume all that based on the code that's there. I'm interested in seeing some example usage. I don't think you're doing anything particularly bizarre, so I'm trying to understand why this code is so bizarre to me. \$\endgroup\$ – nhgrif Apr 5 '16 at 11:57
0
\$\begingroup\$

In looking at your code, what I see are a series of effectively global functions that "make" things based on a string.

The object that calls factoryFor must know what the string is in order to know which type of maker to return (after all, if the string is going to be "X", then the object that calls factoryFor has to know not to pass Shape.) Also, the caller of the factory function must know what the factory is making in order to correctly cast the AnyObject that is returned.

This leads me to wonder what the MakerHelper.factoryFor function's responsibility is? I mean, it's job just seems to be to return the make global function that the caller already knows it wants, so why not just have the caller use the make function directly?

It seems to me that you could have written:

import UIKit

func tintedImageViewNamed(name: String) -> UIImageView {
    let image = UIImage(named: name)
    let tintedImage = image?.imageWithRenderingMode(.AlwaysTemplate)
    let imageView = UIImageView(image: tintedImage)
    imageView.tintColor = UIColor.redColor()
    return imageView
}

func make(type: String) -> UIView? /* thanks nhgrif */ {
    switch type {
    case "CIRCLE":
        //return CircleView()
        return UIView()
    case "X":
        return tintedImageViewNamed("close.png")
    default:
        return nil
    }
}

The above accomplishes the same thing your code does, is no less safe and no more testable than what you have and it's a lot less complex (which means less chance of bugs.)

I'd love to suggest a better solution, but to do that, I would have to better understand what problem you are trying to solve...

\$\endgroup\$
  • \$\begingroup\$ Your AnyObject return should probably be UIView. And while you're right that this code isn't necessarily that much worse, it's not any better, and the code has a big issue to begin with. Namely... consider the scalability of the make method. \$\endgroup\$ – nhgrif Apr 5 '16 at 12:00
  • \$\begingroup\$ Exactly! In the OP's code adding a case requires, at worst, investigating three separate switch statements. The only "improvement" I did was to wrap all three switch statements into one which is far more scaleable. As I said though, there is probably a better solution, but I don't know what the problem to solve is so I can't discover it. \$\endgroup\$ – Daniel T. Apr 5 '16 at 13:57
  • \$\begingroup\$ Which is exactly why the question is on hold and why I would've recommended not answering it at all. \$\endgroup\$ – nhgrif Apr 5 '16 at 14:00
  • \$\begingroup\$ Yea, I'm new here and still learning. I answered before it got put on hold (obviously) and didn't realize at the time that answering was inappropriate. \$\endgroup\$ – Daniel T. Apr 5 '16 at 17:31

Not the answer you're looking for? Browse other questions tagged or ask your own question.