Code Review Stack Exchange is a question and answer site for peer programmer code reviews. Join them; it only takes a minute:

Sign up
Here's how it works:
  1. Anybody can ask a question
  2. Anybody can answer
  3. The best answers are voted up and rise to the top

I am familiar with using scoreboards to construct UI elements in Xcode, but I am only beginning to learn how to make UI programmatically (with out storyboard).

I wanted to know if this is the correct way to implement a UIView into an existing ViewController class.

DetailView (Custom UIView class):

.h file (header)

@interface DetailView : UIView

@property (strong, nonatomic) UIImageView *imageView;
@property (strong, nonatomic) UIImageView *ratingView;
@property (strong, nonatomic) UILabel *tagLabel;
@property (strong, nonatomic) UILabel *plotLabel;


@end

.m file (implementation)

- (instancetype)init {

if(self = [super init]){
    // Creating the large movie image
    _imageView = [[UIImageView alloc] initWithFrame:CGRectMake(0.0, 64.0, 375.0f, 200.0f)];

    // Creating the rating image
    _ratingView = [[UIImageView alloc] init];

    // Creating the tag Label
    _tagLabel = [[UILabel alloc] initWithFrame:CGRectMake(38, 274, 355, 200)];
    self.tagLabel.font =  [UIFont fontWithName:@"HelveticaNeue" size:12];

    // Creating the plot label
    _plotLabel = [[UILabel alloc] initWithFrame:CGRectMake(10, 299, 355, 200)];
    self.plotLabel.font = [UIFont fontWithName:@"HelveticaNeue" size:14];


    [self addSubview:self.imageView];
    [self addSubview:self.ratingView];
    [self addSubview:self.tagLabel];
    [self addSubview:self.plotLabel];
}

return self;
}

View Controller:

Commented code is another way I attempted to implement the DetailView

- (void)viewDidLoad {
[super viewDidLoad];
self.detailView = [[DetailView alloc] init];

self.title = self.movie.title;
self.view.backgroundColor = [UIColor whiteColor];

self.detailView.imageView.image =
[UIImage imageWithData:[NSData dataWithContentsOfURL:[NSURL URLWithString:self.movie.fanArt]]];
//[self.view addSubview:self.detailView.imageView];

[self setRatingImage:self.movie.rating];
//[self.view addSubview:self.detailView.ratingView];


self.detailView.tagLabel.numberOfLines = 0;
self.detailView.tagLabel.text = self.movie.tagline;
[self.detailView.tagLabel sizeToFit];
//[self.view addSubview:self.detailView.tagLabel];

self.detailView.plotLabel.numberOfLines = 0;
self.detailView.plotLabel.text = self.movie.plot;
[self.detailView.plotLabel sizeToFit];
//[self.view addSubview:self.detailView.plotLabel];


[self.view addSubview:self.detailView];
}
share|improve this question
    
Would you mind adding some detail as to why storyboards won't work for you? – nhgrif Oct 24 '14 at 13:25
1  
@nhgrif they work perfectly, I am just learning how to do it with code so I have a better understanding. – Jacob Young Oct 24 '14 at 15:22
up vote 2 down vote accepted

A couple of thoughts:

  1. It is curious to do some of the configuration of the subview's within your custom class (e.g. the frames) and other portions (e.g. numberOfLines and sizeToFit) in the view controller. If I were going to have this UIView subclass, then I'd encapsulate all the presentation details within that class. As it is, if the UI isn't quite right, you'll never quite know which class you have to take a look at.

  2. It is curious to use numberOfLines of zero, but then to use hard-coded frame sizes. (Sure, you sizeToFit, but don't adjust the origin.) If one label changes size, wouldn't you want to move the other controls accordingly? You might contemplate specifying preferredMaxLayoutWidth, too.

  3. In this day and age of differing device dimensions, it is curious to see hard-coded frame sizes at all. I'd probably implement auto layout unless you had some compelling reason not to. And even if I didn't use auto layout, I'd still specify the autosizing masks.

  4. I generally give my subviews weak references because the process of doing addSubview already maintains a strong reference, and it's not clear your really need two strong references. (Clearly, you have to temporarily use a local variable when instantiating the label, if you go this route.)

    Clearly you're not contemplating it at this point, but let's say you ever wanted to do [self.tagLabel removeFromParentView]. Would you also want to have to remember to do self.tagLabel = nil as well?

    It's not a big deal, but I tend to follow the NIB/storyboard model that I don't generally maintain strong references to subviews: The only strong reference I'd maintain would be to the root view.

  5. You're never setting the frame for your ratingView. I might confirm the clipping and content mode, too.

  6. If you're going to have this UIView subclass, it doesn't feel like the view controller should be playing around with which image to show in the rating view. The view controller should probably just tell the UIView subclass what the rating is, and that subclass should take care of the presentation of the appropriate rating image.

  7. Rather than using fixed fonts, if this was an iOS 7+ target, I'd consider using Dynamic Type.

  8. If [NSURL URLWithString:self.movie.fanArt] is in the local file system, you can get away with doing that main thread, but if it's a remote URL, then definite retrieve that image asynchronously.

  9. In terms of having custom UIView subclass at all, I'm not sure there's enough utility in this subview to warrant a subclass. Perhaps, once you start incorporating the above points, there might be enough substance to make this worth while, but I'm not sure your existing code sample justifies it.

share|improve this answer

Your Answer

 
discard

By posting your answer, you agree to the privacy policy and terms of service.

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