Does this code follow standard Objective-C and MVC conventions? [closed]

Question

This question is unlikely to help any future visitors; it is only relevant to a small geographic area, a specific moment in time, or an extraordinarily narrow situation that is not generally applicable to the worldwide audience of the internet. For help making this question more broadly applicable, visit the help center.

Closed 8 years ago.

I have written a basic app in Objective-C which displays a users contacts with pictures. The issue is, there is no compartmentalization and I feel I am violating certain standards.

Everything is handled through one main ViewController, which begins the work mainly after a user taps a button (through an outlet to IB).

//
//  ViewController.m
//  Future Take 4
//
//  Created by Rooz Mahdavian on 5/31/13.
//  Copyright (c) 2013 Rooz Mahdavian. All rights reserved.
//

#import "ViewController.h"
#import <AddressBook/AddressBook.h>
#import <AddressBookUI/AddressBookUI.h>
#import <QuartzCore/QuartzCore.h>

@interface ViewController ()

@end

@implementation ViewController

- (void)viewDidLoad
{
    [super viewDidLoad];
    // Do any additional setup after loading the view, typically from a nib.
    UITapGestureRecognizer *singleTap = [[UITapGestureRecognizer alloc] initWithTarget:self action:@selector(userAllowedForAccess)];
    singleTap.numberOfTapsRequired = 1;
    singleTap.numberOfTouchesRequired = 1;
    [[self.view viewWithTag:300] addGestureRecognizer:singleTap];
    [[self.view viewWithTag:300] setUserInteractionEnabled:YES];

    CGRect tempButton = [self.view viewWithTag:300].frame;
    tempButton.origin.y = self.view.frame.size.height/2 - 56;
    [self.view viewWithTag:300].frame = tempButton;

    [UIView beginAnimations:nil context:nil];
    [UIView setAnimationDuration: .4];
    [UIView setAnimationDelay: 0];
    [UIView setAnimationCurve:UIViewAnimationCurveEaseOut];

    CGRect tempButton2 = [self.view viewWithTag:300].frame;
    tempButton2.origin.y += 90;
    [self.view viewWithTag:300].frame = tempButton2;

    [self.view viewWithTag:100].alpha = 1;

    [UIView commitAnimations];


}

- (void)didReceiveMemoryWarning
{
    [super didReceiveMemoryWarning];
    // Dispose of any resources that can be recreated.
}


- (IBAction)userAllowedForAccess {
    NSLog(@"User Has Verified Contacts");


    UIView *header = [[UIView alloc] initWithFrame:CGRectMake(0,0, 320, 55)];
    header.backgroundColor = [UIColor colorWithRed:34/255.0 green:34/255.0 blue:34/255.0 alpha:1];

    [header.layer setShadowColor:[UIColor colorWithRed:14/255.0 green:14/255.0 blue:14/255.0 alpha:.5].CGColor];
    [header.layer setShadowOpacity:0];
    [header.layer setShadowRadius:3.0];
    [header.layer setShadowOffset:CGSizeMake(2.0, 2.0)];

    [self.view addSubview:header];
    [self.view bringSubviewToFront:[self.view viewWithTag:100]];

    [UIView beginAnimations:nil context:nil];
    [UIView setAnimationDuration: .7];
    [UIView setAnimationDelay: 0];


    [UIView setAnimationCurve:UIViewAnimationCurveEaseOut];

    CGRect tempLogo = [self.view viewWithTag:100].frame;
    tempLogo.origin.y = -14;
    tempLogo.size.height = tempLogo.size.height/2.5;
    tempLogo.size.width = tempLogo.size.width/2.5;
    tempLogo.origin.x = 160 - tempLogo.size.width/2;
    [self.view viewWithTag:100].frame = tempLogo;

    CGRect tempButton = [self.view viewWithTag:300].frame;
    tempButton.size.width = tempButton.size.width * 2.5;
    tempButton.size.height = tempButton.size.height * 2.5;
    tempButton.origin.x = -((tempButton.size.width/2)-((tempButton.size.width * 2.5))/2);

    [self.view viewWithTag:300].frame = tempButton;
    [self.view viewWithTag:300].alpha = 0;


    [self.view viewWithTag:200].alpha = 0;
    [header.layer setShadowOpacity:1];

    [UIView commitAnimations];





    ABAddressBookRef addressBook = ABAddressBookCreate();
    UIScrollView *scroll = [[UIScrollView alloc] initWithFrame:CGRectMake(0, 60, self.view.frame.size.width, self.view.frame.size.height)];




    __block BOOL accessGranted = NO;
    if (ABAddressBookRequestAccessWithCompletion != NULL) { // we're on iOS 6
        dispatch_semaphore_t sema = dispatch_semaphore_create(0);
        ABAddressBookRequestAccessWithCompletion(addressBook, ^(bool granted, CFErrorRef error) {
            accessGranted = granted;
            dispatch_semaphore_signal(sema);
        });
        dispatch_semaphore_wait(sema, DISPATCH_TIME_FOREVER);
    }


    CFArrayRef allPeople = ABAddressBookCopyArrayOfAllPeople( addressBook );
    CFIndex nPeople = ABAddressBookGetPersonCount( addressBook );


    int peopleInARow = 0;
    int maxPeopleInARow = 3;
    int positionFromTop = 20;
    int IMAGE_SIZE = 70;
    float animationOffset = .5;
    float animationOffsetChange = .3;
    float animationDuration = .5;
    int scaleOffset = 40;
    int screenWidth = 320;
    int startingPositionFromLeft = 26;
    int positionFromLeft = startingPositionFromLeft;
    int topOffset = 40;
    int leftOffset = 26;
    int numberOfRows = 0;


    UIView *contactContainer;
    UIImage* image;
    CALayer * l;
    NSString* name;
    NSString* lastName;
    NSString* firstName;
    UIImageView *newimageview;
    UILabel *label;
    UIView *contactImageContainer;



    for ( int i = 0; i < nPeople; i++ )
    {
        ABRecordRef person = CFArrayGetValueAtIndex( allPeople, i );
        firstName = (__bridge_transfer NSString*)ABRecordCopyValue(person,
                                               kABPersonFirstNameProperty);
        lastName = (__bridge_transfer NSString*)ABRecordCopyValue(person,
                                               kABPersonLastNameProperty);
         name = [NSString stringWithFormat:@"%@ %@", firstName, lastName];





        if(ABPersonHasImageData(person)){
            //NSLog(@"%@", name);
            image = [UIImage imageWithData:(__bridge NSData *)ABPersonCopyImageData(person)];

            //NSLog(@"Position %i", positionFromLeft);
            newimageview = [[UIImageView alloc] initWithFrame:CGRectMake(-scaleOffset/2, -scaleOffset/2, IMAGE_SIZE+scaleOffset, IMAGE_SIZE+scaleOffset)];
            newimageview.contentMode = UIViewContentModeScaleAspectFit;




            [newimageview setImage: image];


            contactContainer = [[UIView alloc] initWithFrame:CGRectMake(positionFromLeft, positionFromTop + 20, IMAGE_SIZE, 200)];
            contactImageContainer = [[UIView alloc] initWithFrame:CGRectMake(0, 0, IMAGE_SIZE, IMAGE_SIZE)];
            contactImageContainer.clipsToBounds = YES;

            l = [contactImageContainer layer];
            [l setMasksToBounds:YES];
            [l setCornerRadius:IMAGE_SIZE/2];

            // You can even add a border
            [l setBorderWidth:0.0];
            [l setBorderColor:[[UIColor colorWithRed:234.0/255.0 green:234.0/255.0 blue:234.0/255.0 alpha:.6] CGColor]];


            [contactImageContainer addSubview:newimageview];







            [contactContainer addSubview:contactImageContainer];

            label =  [[UILabel alloc] initWithFrame: CGRectMake(0, IMAGE_SIZE + 10, IMAGE_SIZE, 20)];
            label.text = firstName;
            label.backgroundColor = [UIColor colorWithRed:0/255.0 green:0/255.0 blue:0/255.0 alpha:0];
            label.textColor = [UIColor whiteColor];
            [label setTextAlignment:NSTextAlignmentCenter];
            [label setFont:[UIFont fontWithName:@"Arial-BoldMT" size:14]];
            [contactContainer addSubview:label];


            contactContainer.alpha = 0;






            [UIView beginAnimations:nil context:nil];
            [UIView setAnimationDuration:animationDuration];
            [UIView setAnimationDelay:animationOffset];
            animationOffset+= animationOffsetChange;
            [UIView setAnimationCurve:UIViewAnimationCurveEaseOut];

            contactContainer.alpha = 1;
            CGRect temp = contactContainer.frame;
            temp.origin.y = positionFromTop;
            contactContainer.frame = temp;

            [UIView commitAnimations];

            if(peopleInARow >= 2){
                positionFromTop += IMAGE_SIZE + topOffset;
                peopleInARow = 0;
                positionFromLeft = startingPositionFromLeft;
                numberOfRows++;

            } else {
                peopleInARow += 1;
                positionFromLeft += IMAGE_SIZE + leftOffset;
            }



            [scroll addSubview:contactContainer];
            [scroll bringSubviewToFront:contactContainer];


        }

    }

    NSLog(@"%i", numberOfRows);

    scroll.contentSize = CGSizeMake(screenWidth, 150 * numberOfRows);
    scroll.pagingEnabled = NO;

    [self.view addSubview:scroll];
    [self.view bringSubviewToFront:scroll];


}

- (IBAction)userDeniedAccess:(id)sender {
    NSLog(@"Denied");
}

@end

Can anyone tell me the best way to rewrite this and what standard/conventions (probably in MVC) I am doing wrong?

Answer 1

You're not violating MVC at any thing. The view controller loads some contacts and displays them in the view. The model, represented by the contact, and the views have no coupling at all. That's correct from MVC's point of view.

As you said, this is a simple app. You may need to add some design to your code if the applications gets bigger and more complicated. But this design is good for such level of complexity. For example, if this class get bigger and more complex, you may separate the contacts loading part into a new class.

However, there's a very bad thing about your code. The method userAllowedForAccess contains more than 100 lines of code. Modularity must be maintained in the level of classes as well as methods and functions. Separate the code to many methods, depending on the function it performs. For example:

UIView *header = [[UIView alloc] initWithFrame:CGRectMake(0,0, 320, 55)];
header.backgroundColor = [UIColor colorWithRed:34/255.0 green:34/255.0 blue:34/255.0 alpha:1];

[header.layer setShadowColor:[UIColor colorWithRed:14/255.0 green:14/255.0 blue:14/255.0 alpha:.5].CGColor];
[header.layer setShadowOpacity:0];
[header.layer setShadowRadius:3.0];
[header.layer setShadowOffset:CGSizeMake(2.0, 2.0)];

[self.view addSubview:header];
[self.view bringSubviewToFront:[self.view viewWithTag:100]];

[UIView beginAnimations:nil context:nil];
[UIView setAnimationDuration: .7];
[UIView setAnimationDelay: 0];


[UIView setAnimationCurve:UIViewAnimationCurveEaseOut];

Can be extracted in a method called - (void) setupHeaderView and you can make another method to load the contacts - (NSArray *) getContacts or just - (NSArray *) contacts to match Objective-C's style for getters definition.

Answer 2

I would start refactoring your

- (IBAction)userAllowedForAccess

method in smaller and atomic methods. It's usually a bad idea to have one method that does basically everything.