2
\$\begingroup\$

I have an app which uses Stack Views to update its UI depending on the number of pigs. For example below, 1 pig and 4 pigs.

enter image description here enter image description here

I set up Stack Views in Storyboard and set isHidden to true based on how many pigs there are. I set up my custom view with my class below and set up x and y values of labels based on the current frame.

class PiggyBankView: UIView {

let titleLbl = UILabel()
let imageView = UIImageView()
let budgetLbl = CountingLabel()
let padding : CGFloat = 10.0
var parentFrame : CGRect?
var mainVC : MainVC?

init(parent : CGRect, frame : CGRect) {
    self.parentFrame = parent
    super.init(frame : frame)
    self.addCustomView()
}

override init(frame: CGRect) {
    super.init(frame: frame)
    self.addCustomView()
}


required init(coder aDecoder: NSCoder) {
    fatalError("init(coder:) has not been implemented")
}

func addCustomView() {

    self.backgroundColor = UIColor.clear

    //Set Padding For Image View
    let pad = padding * 2
    let innerSize = CGSize(width: self.frame.size.width - pad, height: self.frame.size.height - pad)
    let innerFrame = CGRect(x: padding, y: padding, width: innerSize.width, height: innerSize.height)

    //Image View
    imageView.frame = innerFrame
    imageView.backgroundColor = UIColor.clear
    imageView.contentMode = .scaleAspectFit
    imageView.image = UIImage(named: "pig-main")
    let centerPoint = CGPoint(x: imageView.frame.size.width / 2, y: imageView.frame.size.height / 2)

    // Title Label and Budget Label Based on its frame
    let size = CGSize(width: self.frame.size.width - pad, height: self.frame.size.height / 10)

    //Test
    self.setNeedsLayout()
    self.layoutIfNeeded()
    print("HEIGHT : \(self.frame.size.height), WIDTH : \(self.frame.size.width)")


    var parentHeight : CGFloat = 0.0
    var parentWidth : CGFloat = 0.0

    if let mainFrame = parentFrame {
        print(mainFrame.size.height)
        print(mainFrame.size.width)
        parentHeight = mainFrame.size.height
        parentWidth = mainFrame.size.width
    }

    let frameWidth = floor(self.frame.size.width)
    let frameHeight = floor(self.frame.size.height)
    var titleFrame : CGRect
    var labelFrame : CGRect
    var dragFromFrame : CGRect

    print("\(parentWidth / 2)")
    //TALL HEIGHT WIDE WIDTH
    if frameHeight == parentHeight && frameWidth == parentWidth {
        print("1 TALL HEIGHT / WIDE WIDTH ")
        titleFrame = CGRect(x: padding, y: (size.height * 2) - 20, width: size.width, height: size.height)
        labelFrame = CGRect(x: padding, y: (size.height * 7) + 25, width: size.width, height: size.height)



    // TALL HEIGHT SMALL WIDTH
    }else if frameHeight == parentHeight && frameWidth == floor(parentWidth / 2) {
        print("2 TALL HEIGHT / SMALL WIDTH")
        titleFrame = CGRect(x: padding, y: size.height * 3, width: size.width, height: size.height)
        labelFrame = CGRect(x: padding, y: size.height * 6 + 15, width: size.width, height: size.height)


    // SHORT HEIGHT - WIDE WIDTH
    }else if frameHeight == floor(parentHeight / 2) && frameWidth == parentWidth {
        print("3 SHORT HEIGHT / WIDE WIDTH Pig")
        titleFrame = CGRect(x: padding, y: -10, width: size.width, height: size.height)
        labelFrame = CGRect(x: padding, y: size.height * 9 + 10, width: size.width, height: size.height)


    // SHORT HEIGHT - SMALL WIDTH
    }else {
        print("4 SHORT HEIGHT / SHORT WIDTH Pig")
        titleFrame = CGRect(x: padding, y: (size.height * 2) - 10, width: size.width, height: size.height)
        labelFrame = CGRect(x: padding, y: (size.height * 7) + 15, width: size.width, height: size.height)

    }

    // Title label
    titleLbl.frame = titleFrame
    titleLbl.textAlignment = .center
    titleLbl.backgroundColor = UIColor.clear
    titleLbl.font = UIFont(name: "CaviarDreams", size: 20.0)
    titleLbl.textColor = UIColor.black
    titleLbl.adjustsFontSizeToFitWidth = true

    //Price Label
    let budgetFrame = CGRect(x: labelFrame.origin.x, y: labelFrame.origin.y, width: labelFrame.width * 0.8, height: labelFrame.height)
    budgetLbl.frame = budgetFrame
    budgetLbl.center = CGPoint(x: self.frame.width / 2, y: budgetFrame.origin.y)
    budgetLbl.textAlignment = .center
    budgetLbl.backgroundColor = UIColor.clear
    budgetLbl.font = UIFont(name: "Roboto", size: 16.0)
    budgetLbl.textColor = UIColor.black
    budgetLbl.adjustsFontSizeToFitWidth = true

    self.addSubview(imageView)
    self.addSubview(titleLbl)
    self.addSubview(budgetLbl)
    self.addSubview(coinImageView)
    imageView.addSubview(dragFromView)
}

This gets the job done but I feel like my code is messy and unnecessary. Is there a better way to do this?

\$\endgroup\$

1 Answer 1

2
\$\begingroup\$

Welcome to Code Review, and yes! There is room for improvement here :)

First of all, I agree that this is worthy of subclassing UIView - there's enough here that I think makes sense to wrap these up into a custom component.

Property access level and names

When you're creating a custom view you want to keep all the subviews private. Otherwise some external view can grab a reference and do things like myPiggy.titleLabel.removeFromSuperview() instead, set these as private so they're only available from within the scope of your view.

I'd suggest not abbreviating the names. It's easier to read when it's a full word, and Xcode will autocomplete the properties so it's not like it's any faster to type by having shorter names.

class PiggyBankView: UIView {

    private let titleLabel = UILabel()
    private let imageView = UIImageView()
    private let budgetLabel = CountingLabel()

    //...
}

I'm assuming that somewhere in you're view controller you're setting the label text, and possibly pig color through these properties.

Instead, I'd pass this information in through the an initializer :

init(title: String, budget: String, pigImage: UIImage) {
        self.title = title
        // etc
}

Views should only be concerned about their subviews

You're keeping reference to the parent frame and the containing view controller:

//var parentFrame : CGRect?
//var mainVC : MainVC? // (I don't see this actually being used anywhere anyway)

And later in addCustomView() you're using the size of the parent frame to drive the layout logic.

if let mainFrame = parentFrame {
    parentHeight = mainFrame.size.height
    parentWidth = mainFrame.size.width
}

Views should never have to worry about that, and instead the containing view should have the responsibility for setting the size, position etc...

Make use of constraints and/or the view lifecycle

Without seeing how these views are using within the containing view controller I don't actually understand how these views are successfully resizing by toggling their isHidden property in a stack view (unless perhaps you're manually calling addCustomView() from the view controller but even then I'm not sure..)

Instead, I'd suggest either:

  1. Defining constraints that allow the view to be adaptive (e.g. pin the pigImage to the left, right anchors of the view).
  2. overriding layoutSubviews() to manually calculate and update the frames of your subviews as the frame of this view changes.

Personally I prefer constraints approach, but some find it off-putting because you can end up with a lot of verbose code.

I'm pretty sure that you will be able to set some relatively simple constraints that will greatly simplify your list of if/else statements that are checking for different combinations of wide/narrow/tall/short.

With the constraints set up, you'll also get the option of being able to set up a nice animation (if you want) of this view resizing in the stack view as different piggybank views are hidden/displayed.

If these PiggyBankViews are being managed by a UIStackView you'll also probably want to override intrinsicContentSize. I tend to end up using stack views with a UIStackViewDistribution.fillProportionally in which case the intrinsic content size is really used as a reference for the aspect ratio of the view, in your case the PiggyBankView is taller than wide because of the labels above and below the image so I'd return something like this:

override var intrinsicContentSize: CGSize {
        return CGSize(width: 100, height: 130)
}   

which is a pointer for the stack view to set the height of the view to be 130% of the width that it's allocated.

Formatting

This may seem insignificant now, and you're mostly consistent, but in a few places you're adding a space before the colon that separates the property name and type:

// instead of this 
var titleFrame : CGRect

//do this
var titleFrame: CGRect

This is all pretty general feedback, but I hope it inspires you to go out and learn some more about views either for this or a future project!

\$\endgroup\$

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge you have read our privacy policy.

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