Skip to content

Conversation

@DT-1236
Copy link

@DT-1236 DT-1236 commented Jun 19, 2019

The smaller change is that this exposes whether to include thumbnails as a boolean argument. This does end up changing the signature slightly

The search changes should emulate contact searching pretty closely. The partial number matching in contacts only searches the usual breakpoints from the beginning. For example, (123) 456 - 7890, a search for 56 will not match, but 45 will. This change just does full substring searching

I chose to primarily use vanilla for loops since https://stackoverflow.com/questions/992901/how-do-i-iterate-over-an-nsarray suggested that there was a speed benefit to doing it that way. I'm considering changing the outermost enumeration, but the speed seems pretty good as is

@DT-1236 DT-1236 requested review from CoreyLoose and tberman June 19, 2019 18:02
@DT-1236
Copy link
Author

DT-1236 commented Jun 19, 2019

Once we actually switch to using this, the call to Contacts.getContactsMatchingString in AccountSearch will need to get updated, and the call to Contacts.getPhotoForId in FundTargetListItem will need to be removed

[fetchRequest setUnifyResults:YES];

NSError *regExError = nil;
NSRegularExpression *wordRegex = [NSRegularExpression regularExpressionWithPattern:@"\\w+"
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

you may want to check these errors and precreate them as part of the module init.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So if I'm understanding this correctly, variable provided as the error argument in NSRegularExpression is filled with an error object should one get thrown during the operation. Are you suggesting that I make this regular expression in the init and return early if an error gets created?

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, it is always the same, no reason to constantly create it.

NSDictionary *contactDictionary = [self contactToDictionary:obj withThumbnails:NO];
[contacts addObject:contactDictionary];

NSArray *nameKeys = @[
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

These arrays can be pre-created since they are no longer a simple literal.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

When you say pre-created, do you mean construct them in the init so they don't need to get recreated every time the method gets called?

I'll definitely change the mutable array into a vanilla NSArray though

@DT-1236 DT-1236 requested a review from tberman July 25, 2019 22:23
@DT-1236
Copy link
Author

DT-1236 commented Jul 25, 2019

I think this should run better. For some reason I couldn't directly declare the statics with literals. Also, I think it's crazy weird that dispatch_once_t is in snake case

Copy link

@tberman tberman left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this generally looks good, how much have you tested this?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants