5
\$\begingroup\$

I have a view controller on my app, where the user is shown a table view. The table view displays their address book contacts and allows the user to select any friends that they would like to invite to the app.

I have 2 model classes: HALAddressBook represents the address book, and HALContact represents an individual contact.

HALAddressBook.h:

#import <AddressBook/AddressBook.h>
#import <AddressBookUI/AddressBookUI.h>
#import <AddressBook/ABAddressBook.h>
#import <AddressBook/ABPerson.h>

@interface HALAddressBook : NSObject

#pragma mark - Properties
@property NSArray *contacts;

#pragma mark - Instance Methods
- (BOOL)isAccessGranted;

@end

HALAddressBook.m:

#import "HALAddressBook.h"

@implementation HALAddressBook

- (BOOL)isAccessGranted
{

    ABAddressBookRef m_addressbook =  ABAddressBookCreateWithOptions(NULL, NULL);

    __block BOOL accessGranted = NO;
    if (ABAddressBookRequestAccessWithCompletion != NULL) {
        dispatch_semaphore_t sema = dispatch_semaphore_create(0);
        dispatch_async(dispatch_get_global_queue(DISPATCH_QUEUE_PRIORITY_DEFAULT, 0), ^{
            @autoreleasepool {
                // Write your code here...
                // Fetch data from SQLite DB
            }
        });


        ABAddressBookRequestAccessWithCompletion(m_addressbook, ^(bool granted, CFErrorRef error)
        {
          accessGranted = granted;

          dispatch_semaphore_signal(sema);
                        });

        dispatch_semaphore_wait(sema, DISPATCH_TIME_FOREVER);
    }

    if (accessGranted) {
        // Access has been granted
       self.contacts = (__bridge NSArray *)(ABAddressBookCopyArrayOfAllPeople(m_addressbook));

        return YES;

    } else {
    // Access has not been granted
    return NO;
    }
}
@end

HALContact.h:

@interface HALContact : NSObject

#pragma mark - Properties
@property NSArray *phoneNumbers;
@property NSString *mainPhoneNumber;
@property NSString *firstName;
@property ABRecordRef contactRef;

#pragma mark - Instance Variables
- (BOOL)hasMultiplePhoneNumbers;

@end

HALContact.m:

#import "HALContact.h"

@interface HALContact ()

@end

@implementation HALContact

- (BOOL)hasMultiplePhoneNumbers
{
    if (self.phoneNumbers.count > 1) {
        return YES;
    } else {
        return NO;
    }
}

@end

Both of these model classes are then imported into HALAddFriendsViewController.m, and here is how they are used:

- (void)viewDidAppear:(BOOL)animated
{

    [super viewDidAppear:NO];

    // Create an instance of HALAddressBook
    HALAddressBook *addressBook = [[HALAddressBook alloc]init];

    // Ask for access to the user's address book
    BOOL result = [addressBook isAccessGranted];

    if (!result) {
        [self performSegueWithIdentifier:@"addFriendsToMediaCaptureSegue" sender:self];
    }

    // Access has been granted
    // Create array of all address book contacts
    NSArray *contacts = [addressBook contacts];


    // Initialize our friends array
    self.friends = [[NSMutableArray alloc]init];


    // Loop through all of the contacts.
    // Create an instance of HALContact
    // Set the instance's contactRef, firstName and phoneNumbers properties


    for (id currentContact in contacts) {
        // Create an instance of HALContact,
        // set its contactRef property,
        // set its firstName property,
        HALContact *contact = [[HALContact alloc]init];
        contact.contactRef = (__bridge ABRecordRef)(currentContact);
        contact.firstName = (__bridge_transfer NSString
                             *)ABRecordCopyValue(contact.contactRef, kABPersonFirstNameProperty);

        // If firstName is nil, continue
        if (!contact.firstName) continue;

       // Grab all of the phone number values for this contact
       NSArray *phoneNumberValues =  (__bridge NSArray *)(ABRecordCopyValue(contact.contactRef, kABPersonPhoneProperty));

        // Place a copy of all phone numbers into contact's
        // phoneNumber property
        contact.phoneNumbers = (__bridge NSArray *)(ABMultiValueCopyArrayOfAllValues((__bridge ABMultiValueRef)(phoneNumberValues)));


        // Check if the contact has multiple phone numbers
        BOOL result = [contact hasMultiplePhoneNumbers];

        // If contact only has one phone number, then add the
        // number from the phoneNumbers property into the
        // contact's mainPhoneNumber property
        if (!result) {
            contact.mainPhoneNumber = contact.phoneNumbers[0];
            // Add contact to friends array
            [self.friends addObject:contact];
            continue;
        }
            // Loop through the contacts phone numbers
            // and for every phone number, create a copy of the contact
            // and place the current phone number at index in
            // the copy's mainPhoneNumber property.

            // So if "Steve" has 2 phone numbers, he'll be added to our
            // friends array twice, once for each phone number.

            for (int index = 0; index < contact.phoneNumbers.count; index++) {
                HALContact *copyOfContact = [[HALContact alloc]init];
                copyOfContact.mainPhoneNumber = contact.phoneNumbers[index];
                copyOfContact.firstName = contact.firstName;

                // Add copyOfContact to the friends array
                [self.friends addObject:copyOfContact];
            }
    }

    // Set table view's datasource and delegate
    self.tableView.dataSource = self;
    self.tableView.delegate = self;

    // Reload table view data
    [self.tableView reloadData];
}

I am looking for an overall review, and want to see if I am understanding MVC and using it in the correct way.

Some specific things I am also concerned about:

  1. Is it ok that I am setting the properties of the HALContact instance in my view controller?
  2. I was told that - (BOOL)hasMultiplePhoneNumbers should also be a property. Is this true? Is it a general best practice to back BOOL methods with a BOOL property?
  3. How is my overall code quality? Everything from comments, style, etc.
\$\endgroup\$
6
\$\begingroup\$

Is it ok that I am setting the properties of the HALContact instance in my view controller?

In general, yes. In specific, however, I really don't like the way mainPhoneNumber works. I also don't like that phoneNumbers is an array--it should be either a set (so numbers aren't duplicated) or perhaps a dictionary (so that the numbers could have labels, and you could duplicate a number--but not a label (key)).

Let's start with mainPhoneNumber and work with the assumption that phoneNumbers will remain an array (which is okay, but not necessarily best).

Rather than a separate variable, why don't we just let mainPhoneNumber be a convenient way of accessing the first object of the phoneNumbers array?

Without changing anything in the header of HALContact or anything anywhere that uses HALContact, we can do that simply by adding the following two methods to HALContact.m:

- (void)setMainPhoneNumber:(NSString *)mainPhoneNumber {
    if (![mainPhoneNumber isEqualToString:[self.phoneNumbers firstObject]]) {
        [self.phoneNumbers removeObject:mainPhoneNumber];
        self.phoneNumbers = [@[mainPhoneNumber] arrayByAddingObjectsFromArray:self.phoneNumbers];            
    }
}

The removeObject: method is optional and depends on whether or not you'd want to allow a duplicated number.

- (NSString *)mainPhoneNumber {
    return [self.phoneNumbers firstObject];
}

Now, there's no longer a backing variable for mainPhoneNumber, but we still used it as a property, so it can still be accessed as a property, and we don't have to waste time setting it--it's essentially set automatically when we set the phoneNumbers array.


I was told that - (BOOL)hasMultiplePhoneNumbers should also be a property. Is this true? Is it a general best practice to back BOOL methods with a BOOL property?

I actually agree that this should probably be a property, HOWEVER, that doesn't necessarily mean there's a backing variable for the property. I just demonstrated how we can have a mainPhoneNumber property without a backing NSString variable.

This one is slightly trickier, as you wouldn't want to allow someone to just set this value.

First, declare the property with the readonly attribute:

@property (nonatomic,assign,readonly) BOOL hasMultiplePhoneNumbers;

This means there's no setter method created. You could technically create one. You can create readonly properties that still have a backing variable, and assign it, but the key is no setter method is created.

Now, all that's left is overriding the getter--which you've already done. And because you didn't reference _hasMultiplePhoneNumbers in the getter and declared the property as readonly, no actual backing variable is declared.

I would though recommend refactoring the getter to look like this:

- (BOOL)hasMultiplePhoneNumbers {
    return (self.phoneNumbers.count > 1);
}

Any method that takes the form:

if (someCondition) {
    return YES;
} else {
    return NO;
}

Can always be refactored to simply:

return someCondition;

How is my overall code quality? Everything from comments, style, etc.

contact.phoneNumbers[0];

When trying to fetch the first object of an array, you should always use the firstObject method. Accessing the first index will crash on an empty array (as well as I think if the array is nil), however firstObject instead simply returns nil and won't crash. Even if you think you can guarantee the object will never be empty, use firstObject for safety.


- (void)viewDidAppear:(BOOL)animated
{

    [super viewDidAppear:NO];

I can't think of any good reason to ever send anything but the animated variable you were passed when calling to super in the view controller life-cycle events.


for (id currentContact in contacts) {

The indentation within the body of this loop seems not quite right. Double check and probably fix that.


for (int index = 0; index < contact.phoneNumbers.count; index++) {
    HALContact *copyOfContact = [[HALContact alloc]init];
    copyOfContact.mainPhoneNumber = contact.phoneNumbers[index];
    copyOfContact.firstName = contact.firstName;

    // Add copyOfContact to the friends array
    [self.friends addObject:copyOfContact];
}

Let's refactor this to a forin loop, which in Objective-C will execute faster than a regular for loop generally:

for (NSString *phoneNumber in contact.phoneNumbers) {
    HALContact *copyOfContact = [[HALContact alloc] init];
    copyOfContact.mainPhoneNumber = phoneNumber;
    copyOfContact.firstName = contact.firstName;
    [self.friends addObject:copyOfContact];
}

self.tableView.dataSource = self;
self.tableView.delegate = self;

If you've designed your view controller in a XIB or storyboard, you can set these in interface builder (it's a right-click option). Otherwise, they should be set in viewDidLoad. We don't need to reset this every time the view appears.


[self.tableView reloadData];

There's nothing wrong with this line... however, ultimately, ALL of this code should probably be in viewWillAppear: (unless there's something I'm missing) so that the table view can have it's data right before the view appears and the user doesn't see an empty table for a second before it reloads with its data.


And ultimately, all of the code you execute in this method should be its own method actually.

So in the broad scope...

Setting delegates/datasources should either be done through interface builder or in viewDidLoad.

All of the rest of the code except the call to super and calling reloadData on the table view should be moved into a separate method.

viewWillAppear: should, in this order:

  1. Make a call to [super viewWillAppear:animated];
  2. Call the method you moved all the contact code into.
  3. Call [self.tableView reloadData];
\$\endgroup\$
  • \$\begingroup\$ I cannot thank you enough. Thank you for taking the time to review everything and provide detailed changes and insight. Seriously, it is greatly appreciated. It took me a couple minutes to figure out your accessor changes to mainPhoneNumber, but now i get it and really like it. \$\endgroup\$ – user3344977 Jul 11 '14 at 18:00
  • \$\begingroup\$ Two questions. 1. Why did you not like the original solution I had for mainPhoneNumber? Can you explain to me why yours is better? 2. You said "The removeObject: method is optional and depends on whether or not you'd want to allow a duplicated number." I'm not sure how removeObject takes care of a duplicate when we're creating a new array with that same number on the next line? \$\endgroup\$ – user3344977 Jul 11 '14 at 18:03
  • \$\begingroup\$ 1. My approach to mainPhoneNumber eliminates duplicate data. 2. We're creating a new array with new "main phone number" and the existing array. If we don't call removeObject:, then the mainPhoneNumber: we just added might be in there twice (in its original non-front position as well as at the front). By calling removeObject: we eliminate it from the existing array, then recreate the array with the phone number at the first index. \$\endgroup\$ – nhgrif Jul 11 '14 at 18:07
  • \$\begingroup\$ To elaborate on the first question, there's a problem if we were to change the array of phone numbers but not change the mainPhoneNumber. Your original code means it'd be possible for mainPhoneNumber to not be included in the phoneNumbers array, which seems odd to me. Your code also means that someone could have 1 or more phone numbers, but no "main phone number" selected. \$\endgroup\$ – nhgrif Jul 11 '14 at 18:09
  • \$\begingroup\$ I understand now. "... which seems odd to me." It was a poor solution. The reason I did that was to create a solution for a friend having multiple phone numbers, and making sure there was a cell for each, but your way is much cleaner. Thanks for the help! \$\endgroup\$ – user3344977 Jul 11 '14 at 18:25

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

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