2
\$\begingroup\$

I started to refactor my app to MVC today. I wasted a lot of time because of simple mistakes.

One was I kept trying to access the getter method inside of my custom getter method, which just causes and endless loop of gets.

Even when I figured it out, I repeated this mistake several times. I think it's safe to say I'll never use self.iVar inside of it's own getter method ever again.

The thing I sort of want advice on is not just whether I did an ok MVC refactor, but whether I approached things the right way.

One of the things that is really bugging me is my implementation of the phoneNumbers getter method in HALContact.

When I first created it, my nested for loops in my New View Controller Snippet would run forever and just keep adding the same phone number instead of moving on.

I finally just got frustrated and added the removeAllObjects method at the beginning of the phoneNumbers getter. As much as it works fine, I'm not sure if that's the way it should be.

And this is how the getter now looks and works fine:

- (NSArray *)phoneNumbers {

    // The difference is I remove all of the objects
    if (_phoneNumbers) {
        [_phoneNumbers removeAllObjects];
    }

    if (!_phoneNumbers) {
        //Create _phoneNumbers array
        _phoneNumbers = [[NSMutableArray alloc]init];
    }

    ABMultiValueRef *phoneNumberRef = ABRecordCopyValue(_contactRef, kABPersonPhoneProperty);

    NSString *phoneNumber = [[NSString alloc] init];

    // Make sure multi value ref exists
    if (phoneNumberRef) {
        CFIndex numberOfPhoneNumbers = ABMultiValueGetCount(phoneNumberRef);

        for (CFIndex i = 0; i < numberOfPhoneNumbers; i++) {
            phoneNumber = (__bridge_transfer NSString *)ABMultiValueCopyValueAtIndex(phoneNumberRef, i);
            CFStringRef label = ABMultiValueCopyLabelAtIndex(phoneNumberRef, i);
            if (label) {

            [_phoneNumbers addObject:phoneNumber];
                NSLog(@"phoneNumbers count in method: %d", _phoneNumbers.count);
            }
            CFRelease(label);
        }
        CFRelease(phoneNumberRef);
    }
    return _phoneNumbers;
}

As far as the general MVC refactor, I threw up all of my new and old files on Pastebin which I will link to in parentheses if you'd rather view them in separate tabs.

Here's a snippet of what the View Controller looks like after the refactor. It now uses 2 model classes I made to handle the address book and contacts data:

// Instantiate our address book instance
self.addressBook = [[HALAddressBook alloc]init];

// Instantiate our contact instance
self.currentContact = [[HALContact alloc]init];

//Ask for access to Address Book.
BOOL result = [self.addressBook requestAccess];

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

if (result) {

    // Loop through all address book contacts
    for (int index = 0; index < self.addressBook.allContacts.count; index++) {

       // Create contactRef from current contact
        self.currentContact.contactRef = (__bridge ABRecordRef)(self.addressBook.allContacts[index]);

      // Loop through all of the current contact's phone numbers
        int index2;
        for (index2 = 0; index2 < self.currentContact.phoneNumbers.count; index2++) {
            if (self.currentContact.firstName) {
                // Add current contact's phone number to array
                [self.potentiaFriendsPhoneNumberArray addObject:self.currentContact.phoneNumbers[index2]];

                // Add current contact's first name to array
                [self.potentiaFriendsNotInParseFirstNamesArray addObject:self.currentContact.firstName];
            }
        }
    }
}

And here are the 2 model classes that I added. One for the address book and one for the contact:

1A. HALAddressBook header

1B. HALAddressBook implementation

2A. HALContact header

2B. HALContact implementation

\$\endgroup\$
2
\$\begingroup\$
if (_phoneNumbers) {
    [_phoneNumbers removeAllObjects];
}

if (!_phoneNumbers) {
    //Create _phoneNumbers array
    _phoneNumbers = [[NSMutableArray alloc]init];
}

Instead of this, we can replace all these lines with simply:

_phoneNumbers = [NSMutableArray array];

The best advantage of this, is we don't run into any problems of emptying an array that someone else might have a reference to. This could cause problems if, for example, you were using the phone numbers array as the data for a table view. If you all of the sudden removed all the objects, but didn't call reloadData on the table view, the next time it tried grabbing a cell, it could be an index out of bounds.


ABMultiValueRef *phoneNumberRef = ABRecordCopyValue(_contactRef, kABPersonPhoneProperty);

NSString *phoneNumber = [[NSString alloc] init];

// Make sure multi value ref exists
if (phoneNumberRef) {
    CFIndex numberOfPhoneNumbers = ABMultiValueGetCount(phoneNumberRef);

    for (CFIndex i = 0; i < numberOfPhoneNumbers; i++) {
        phoneNumber = (__bridge_transfer NSString *)ABMultiValueCopyValueAtIndex(phoneNumberRef, i);
        CFStringRef label = ABMultiValueCopyLabelAtIndex(phoneNumberRef, i);
        if (label) {

        [_phoneNumbers addObject:phoneNumber];
            NSLog(@"phoneNumbers count in method: %d", _phoneNumbers.count);
        }
        CFRelease(label);
    }
    CFRelease(phoneNumberRef);
}

I think this section is problematic. I don't know enough about AddressBook to really tell you for sure, so for now I'm just mentioning it and reserving a section for in the future if I come back and figure out what's wrong with it. The CFRelease on phoneNumberRef seems particularly suspicious.


BOOL result = [self.addressBook requestAccess];

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

if (result) {

This could and probably should be refactored into:

if (![self.addressBook requestAccess]) {
    [self performSegueWithIdentifier:@"addFriendsToMediaCaptureSegue" sender:self];
} else {

But... with that said, I did take a look at the requestAccess method in the link. Strictly speaking, it's not in the question and so it's not strictly up for review, but, I'm quite certain it will always return NO, as the return is synchronous but the variable being returned is only set in an asynchronous block, and as such, will always be set too late.


int index2;
for (index2 = 0; index2 < self.currentContact.phoneNumbers.count; index2++) {
    if (self.currentContact.firstName) {
        // Add current contact's phone number to array
        [self.potentiaFriendsPhoneNumberArray addObject:self.currentContact.phoneNumbers[index2]];

        // Add current contact's first name to array
        [self.potentiaFriendsNotInParseFirstNamesArray addObject:self.currentContact.firstName];
    }
}

First, we can declare our iterator variable in the for loop initialization statement. Remember, you did this a few lines up. But perhaps more importantly, why don't we use a forin?

for (id phoneNumber in self.currentContact.phoneNumbers) {
    [self.potentialFriendsPhoneNumberArray addObject:phoneNumber];
    [self.potentialFriendsNotInParseFirstNamesArray addObject:self.currentContact.firstName];
}

We should also move the if check outside the loop so that the entire loop can be circumvented in the case that we're never going to add anything anyway. Embed the loop within the if.

Also, it doesn't make a ton of sense to me for there to be two separate arrays. I understand that you'll want a name associated with all the phone numbers, however it's better to do this with a single array, either by filling it with dictionaries with a key for the phone number and a key for the name, or by creating a custom object that will hold both of these parts.

\$\endgroup\$
  • \$\begingroup\$ Thank you so much for help. I actually went through my code again before seeing your answer and made a ton of changes, including a lot of the ones you listed. I'm going to open a new question this time with much cleaner code and would love your insight on that as well. \$\endgroup\$ – user3344977 Jul 10 '14 at 18:30
  • \$\begingroup\$ Here's the new refactor, please let me know what you think: codereview.stackexchange.com/questions/56680/… \$\endgroup\$ – user3344977 Jul 10 '14 at 19:11
  • \$\begingroup\$ I'll look at it when I can \$\endgroup\$ – nhgrif Jul 10 '14 at 20:02

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.