0

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?

1

2 Answers 2

3

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.

3
  • okay, I split it up into about 10 different methods. Anything else of note?
    – roozbubu
    Jun 1, 2013 at 17:51
  • 1
    There's a little thing. Why don't use xib or storyboard files to load your views? This can extract loads of code from your class. Jun 1, 2013 at 17:53
  • I use a storyboard to create the base view and some static images, but mostly everything else (the contact views) Is created dynamically. What I can move though is the scroll view and the header - thank you
    – roozbubu
    Jun 1, 2013 at 17:56
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.

3
  • gotcha, did that. broke it up into about 9-10 smaller methods and created global properties to manage things more easily. But it doesn't look there is some huge rule or convention I have violated?
    – roozbubu
    Jun 1, 2013 at 17:43
  • 1
    What you need to understand is basically that you View Controller's role is to manage the interaction between your view(s) and your model(s). You could isolate the Model-related stuff (AddressBook fetch) into another class and maybe also put the animations (which are view-related stuff) into another place.
    – Bruno Koga
    Jun 1, 2013 at 17:46
  • I am moving the AddressBook code into a new dedicated model, but I'm not quite sure where I could move the View related code, as it was created through IB
    – roozbubu
    Jun 1, 2013 at 17:50

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