From 34ffce89f59356ab23f290866b1c3437f03312ce Mon Sep 17 00:00:00 2001 From: Russ Shanahan Date: Tue, 29 Nov 2016 10:02:46 -0500 Subject: [PATCH] Only calculate fullName once, & sortable fullnames (#67) 1. Adds caching of the calculated fullName value (which will slightly improve performance) 2. Incorporates fullNames that respect the first-name-first rules of the currently unmerged PR #22 3. Adds two new fullName properties that can be used for sorting comparators 4. Move the comparator into the model object for easy testing Includes tests to ensure that the first name first and last name first sorts are behaving as expected. // FREEBIE --- .../TSKitiOSTestApp.xcodeproj/project.pbxproj | 4 + src/Contacts/Contact.h | 5 + src/Contacts/Contact.m | 68 ++++++++-- tests/Contacts/ContactSortingTest.m | 119 ++++++++++++++++++ 4 files changed, 189 insertions(+), 7 deletions(-) create mode 100644 tests/Contacts/ContactSortingTest.m diff --git a/Example/TSKitiOSTestApp/TSKitiOSTestApp.xcodeproj/project.pbxproj b/Example/TSKitiOSTestApp/TSKitiOSTestApp.xcodeproj/project.pbxproj index 39cba8f6e..03e5e0167 100644 --- a/Example/TSKitiOSTestApp/TSKitiOSTestApp.xcodeproj/project.pbxproj +++ b/Example/TSKitiOSTestApp/TSKitiOSTestApp.xcodeproj/project.pbxproj @@ -43,6 +43,7 @@ B6273DDF1C13A2E500738558 /* Main.storyboard in Resources */ = {isa = PBXBuildFile; fileRef = B6273DDD1C13A2E500738558 /* Main.storyboard */; }; B6273DE11C13A2E500738558 /* Assets.xcassets in Resources */ = {isa = PBXBuildFile; fileRef = B6273DE01C13A2E500738558 /* Assets.xcassets */; }; B6273DE41C13A2E500738558 /* LaunchScreen.storyboard in Resources */ = {isa = PBXBuildFile; fileRef = B6273DE21C13A2E500738558 /* LaunchScreen.storyboard */; }; + D2AECE731DE8C3360068CE15 /* ContactSortingTest.m in Sources */ = {isa = PBXBuildFile; fileRef = D2AECE721DE8C3360068CE15 /* ContactSortingTest.m */; }; /* End PBXBuildFile section */ /* Begin PBXContainerItemProxy section */ @@ -105,6 +106,7 @@ B6273DF01C13A2E500738558 /* Info.plist */ = {isa = PBXFileReference; lastKnownFileType = text.plist.xml; path = Info.plist; sourceTree = ""; }; B8362AB8E280E0F64352F08A /* libPods-TSKitiOSTestApp.a */ = {isa = PBXFileReference; explicitFileType = archive.ar; includeInIndex = 0; path = "libPods-TSKitiOSTestApp.a"; sourceTree = BUILT_PRODUCTS_DIR; }; C0DC1A83C39CBC09FB2405A3 /* libPods-TSKitiOSTestAppTests.a */ = {isa = PBXFileReference; explicitFileType = archive.ar; includeInIndex = 0; path = "libPods-TSKitiOSTestAppTests.a"; sourceTree = BUILT_PRODUCTS_DIR; }; + D2AECE721DE8C3360068CE15 /* ContactSortingTest.m */ = {isa = PBXFileReference; fileEncoding = 4; lastKnownFileType = sourcecode.c.objc; name = ContactSortingTest.m; path = ../../../tests/Contacts/ContactSortingTest.m; sourceTree = ""; }; D3737F7A041D7147015C02C2 /* Pods-TSKitiOSTestAppTests.release.xcconfig */ = {isa = PBXFileReference; includeInIndex = 1; lastKnownFileType = text.xcconfig; name = "Pods-TSKitiOSTestAppTests.release.xcconfig"; path = "Pods/Target Support Files/Pods-TSKitiOSTestAppTests/Pods-TSKitiOSTestAppTests.release.xcconfig"; sourceTree = ""; }; /* End PBXFileReference section */ @@ -207,6 +209,7 @@ 45B700961D9841E400269FFD /* OWSDisappearingMessagesConfigurationTest.m */, 4516E3E71DD153CC00DC4206 /* TSGroupThreadTest.m */, 4516E3E91DD1542300DC4206 /* TSContactThreadTest.m */, + D2AECE721DE8C3360068CE15 /* ContactSortingTest.m */, ); name = Contacts; sourceTree = ""; @@ -552,6 +555,7 @@ 459850C11D22C6F2006FFEDB /* PhoneNumberTest.m in Sources */, 45458B7A1CC342B600A02153 /* TSStorageSignedPreKeyStore.m in Sources */, 453E1FDB1DA83EFB00DDD7B7 /* OWSFakeContactsUpdater.m in Sources */, + D2AECE731DE8C3360068CE15 /* ContactSortingTest.m in Sources */, 453E1FD81DA83E1000DDD7B7 /* OWSFakeContactsManager.m in Sources */, 454021ED1D960ABF00F2126D /* OWSDisappearingMessageFinderTest.m in Sources */, 4516E3EA1DD1542300DC4206 /* TSContactThreadTest.m in Sources */, diff --git a/src/Contacts/Contact.h b/src/Contacts/Contact.h index 692f35526..13831bed2 100644 --- a/src/Contacts/Contact.h +++ b/src/Contacts/Contact.h @@ -12,12 +12,15 @@ NS_ASSUME_NONNULL_BEGIN @class CNContact; @class PhoneNumber; +@class UIImage; @interface Contact : NSObject @property (nullable, readonly, nonatomic) NSString *firstName; @property (nullable, readonly, nonatomic) NSString *lastName; @property (readonly, nonatomic) NSString *fullName; +@property (readonly, nonatomic) NSString *comparableNameFirstLast; +@property (readonly, nonatomic) NSString *comparableNameLastFirst; @property (readonly, nonatomic) NSArray *parsedPhoneNumbers; @property (readonly, nonatomic) NSArray *userTextPhoneNumbers; @property (readonly, nonatomic) NSArray *emails; @@ -43,6 +46,8 @@ NS_ASSUME_NONNULL_BEGIN #endif // TARGET_OS_IOS ++ (NSComparator)comparatorSortingNamesByFirstThenLast:(BOOL)firstNameOrdering; + @end NS_ASSUME_NONNULL_END diff --git a/src/Contacts/Contact.m b/src/Contacts/Contact.m index 1c9a4977b..2434dac45 100644 --- a/src/Contacts/Contact.m +++ b/src/Contacts/Contact.m @@ -9,6 +9,10 @@ NS_ASSUME_NONNULL_BEGIN @implementation Contact +@synthesize fullName = _fullName; +@synthesize comparableNameFirstLast = _comparableNameFirstLast; +@synthesize comparableNameLastFirst = _comparableNameLastFirst; + #if TARGET_OS_IOS - (instancetype)initWithContactWithFirstName:(nullable NSString *)firstName andLastName:(nullable NSString *)lastName @@ -91,17 +95,54 @@ NS_ASSUME_NONNULL_BEGIN } - (NSString *)fullName { - NSMutableString *fullName = [NSMutableString string]; - if (self.firstName) - [fullName appendString:self.firstName]; - if (self.lastName) { - [fullName appendString:[NSString stringWithFormat:@" %@", self.lastName]]; + if (_fullName == nil) { + if (ABPersonGetCompositeNameFormat() == kABPersonCompositeNameFormatFirstNameFirst) { + _fullName = [self combineLeftName:_firstName withRightName:_lastName usingSeparator:@" "]; + } else { + _fullName = [self combineLeftName:_lastName withRightName:_firstName usingSeparator:@" "]; + } + } + + return _fullName; +} + +- (NSString *)comparableNameFirstLast { + if (_comparableNameFirstLast == nil) { + // Combine the two names with a tab separator, which has a lower ascii code than space, so that first names + // that contain a space ("Mary Jo\tCatlett") will sort after those that do not ("Mary\tOliver") + _comparableNameFirstLast = [self combineLeftName:_firstName withRightName:_lastName usingSeparator:@"\t"]; + } + + return _comparableNameFirstLast; +} + +- (NSString *)comparableNameLastFirst { + if (_comparableNameLastFirst == nil) { + // Combine the two names with a tab separator, which has a lower ascii code than space, so that last names + // that contain a space ("Van Der Beek\tJames") will sort after those that do not ("Van\tJames") + _comparableNameLastFirst = [self combineLeftName:_lastName withRightName:_firstName usingSeparator:@"\t"]; + } + + return _comparableNameLastFirst; +} + +- (NSString *)combineLeftName:(NSString *)leftName withRightName:(NSString *)rightName usingSeparator:(NSString *)separator { + const BOOL leftNameNonEmpty = (leftName.length > 0); + const BOOL rightNameNonEmpty = (rightName.length > 0); + + if (leftNameNonEmpty && rightNameNonEmpty) { + return [NSString stringWithFormat:@"%@%@%@", leftName, separator, rightName]; + } else if (leftNameNonEmpty) { + return [leftName copy]; + } else if (rightNameNonEmpty) { + return [rightName copy]; + } else { + return @""; } - return fullName; } - (NSString *)description { - return [NSString stringWithFormat:@"%@ %@: %@", self.firstName, self.lastName, self.userTextPhoneNumbers]; + return [NSString stringWithFormat:@"%@: %@", self.fullName, self.userTextPhoneNumbers]; } - (BOOL)isSignalContact { @@ -124,6 +165,19 @@ NS_ASSUME_NONNULL_BEGIN return identifiers; } ++ (NSComparator)comparatorSortingNamesByFirstThenLast:(BOOL)firstNameOrdering { + return ^NSComparisonResult(id obj1, id obj2) { + Contact *contact1 = (Contact *)obj1; + Contact *contact2 = (Contact *)obj2; + + if (firstNameOrdering) { + return [contact1.comparableNameFirstLast caseInsensitiveCompare:contact2.comparableNameFirstLast]; + } else { + return [contact1.comparableNameLastFirst caseInsensitiveCompare:contact2.comparableNameLastFirst]; + } + }; +} + @end NS_ASSUME_NONNULL_END diff --git a/tests/Contacts/ContactSortingTest.m b/tests/Contacts/ContactSortingTest.m new file mode 100644 index 000000000..911ed43be --- /dev/null +++ b/tests/Contacts/ContactSortingTest.m @@ -0,0 +1,119 @@ +// Created by Russ Shanahan on 11/25/16. +// Copyright © 2016 Open Whisper Systems. All rights reserved. + +#import "Contact.h" +#import + +NS_ASSUME_NONNULL_BEGIN + +@interface ContactSortingTest : XCTestCase + +@end + +@implementation ContactSortingTest + +- (void)setUp +{ + srandom((unsigned int)time(NULL)); +} + +- (void)testSortingNamesByFirstLast +{ + NSComparator comparator = [Contact comparatorSortingNamesByFirstThenLast:YES]; + NSArray*sortedContacts = [self.class contactArrayForNames:@[@[@"Adam", @"Smith"], + @[@"Adam", @"West"], + @[@"", @"Daisy"], + @[@"Daisy", @"Chain"], + @[@"Daisy", @"Duke"], + @[@"James", @"Smith"], + @[@"James", @"Van"], + @[@"James", @"Van Der Beek"], + @[@"Kevin", @"Smith"], + @[@"Mae", @"West"], + @[@"Mary", @"Oliver"], + @[@"Mary Jo", @"Catlett"], + ]]; + NSUInteger numContacts = sortedContacts.count; + + for (NSUInteger i = 0; i < 20; i++) { + NSArray *shuffledContacts = [self.class shuffleArray:sortedContacts]; + NSArray *resortedContacts = [shuffledContacts sortedArrayUsingComparator:comparator]; + for (NSUInteger j = 0; j < numContacts; j++) { + Contact *a = sortedContacts[j]; + Contact *b = resortedContacts[j]; + BOOL correct = ([a.firstName isEqualToString:b.firstName] && [a.lastName isEqualToString:b.lastName]); + if (!correct) { + XCTAssert(@"Contacts failed to sort names by first, last"); + break; + } + } + } +} + +- (void)testSortingNamesByLastFirst +{ + NSComparator comparator = [Contact comparatorSortingNamesByFirstThenLast:NO]; + NSArray*sortedContacts = [self.class contactArrayForNames:@[@[@"Mary Jo", @"Catlett"], + @[@"Daisy", @"Chain"], + @[@"", @"Daisy"], + @[@"Daisy", @"Duke"], + @[@"Mary", @"Oliver"], + @[@"Adam", @"Smith"], + @[@"James", @"Smith"], + @[@"Kevin", @"Smith"], + @[@"James", @"Van"], + @[@"James", @"Van Der Beek"], + @[@"Adam", @"West"], + @[@"Mae", @"West"], + ]]; + NSUInteger numContacts = sortedContacts.count; + + for (NSUInteger i = 0; i < 20; i++) { + NSArray *shuffledContacts = [self.class shuffleArray:sortedContacts]; + NSArray *resortedContacts = [shuffledContacts sortedArrayUsingComparator:comparator]; + for (NSUInteger j = 0; j < numContacts; j++) { + Contact *a = sortedContacts[j]; + Contact *b = resortedContacts[j]; + BOOL correct = ([a.firstName isEqualToString:b.firstName] && [a.lastName isEqualToString:b.lastName]); + if (!correct) { + XCTAssert(@"Contacts failed to sort names by last, first"); + break; + } + } + } +} + ++ (NSArray *)contactArrayForNames:(NSArray*>*)namePairs +{ +#pragma clang diagnostic push +#pragma clang diagnostic ignored "-Wdeprecated-declarations" + ABRecordID fakeRecordId = 0; +#pragma clang diagnostic pop + NSMutableArray*contacts = [[NSMutableArray alloc] initWithCapacity:namePairs.count]; + for (NSArray*namePair in namePairs) { + Contact *c = [[Contact alloc] initWithContactWithFirstName:namePair[0] + andLastName:namePair[1] + andUserTextPhoneNumbers:@[] + andImage:nil + andContactID:fakeRecordId++]; + [contacts addObject:c]; + } + + return [contacts copy]; // Return an immutable for good hygene +} + ++ (NSArray*)shuffleArray:(NSArray *)array +{ + NSMutableArray *shuffled = [[NSMutableArray alloc] initWithArray:array]; + + for(NSUInteger i = [array count]; i > 1; i--) { + NSUInteger j = arc4random_uniform((uint32_t)i); + [shuffled exchangeObjectAtIndex:(i - 1) withObjectAtIndex:j]; + } + + return [shuffled copy]; // Return an immutable for good hygene +} + +@end + +NS_ASSUME_NONNULL_END