Adding friends to address book app

Question

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.

Answer 1

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];