Feedback for My Challenge Solution


#1

Being from a Microsoft.Net background, I appreciate some feedback on how I solved this problem. TIA

PossessionStore.h

#import <Foundation/Foundation.h>

@class Possession;

@interface PossessionStore : NSObject
{
    NSMutableArray *allPossessions;
    NSMutableArray *possessionsOver50;
    NSMutableArray *possessionsUnder50;
}

+ (PossessionStore *) defaultStore;
- (NSArray *) allPossessions;
- (Possession *) createPossession;
+ (NSMutableArray *) possessionsOver50;
+ (NSMutableArray *) possessionsUnder50;

@end

Added the following to PossessionStore.m

+ (NSArray *) possessionsOver50
{
    NSMutableArray *items = [[NSMutableArray alloc] init];
    
    for (Possession *p in defaultStore.allPossessions)
    {
        if (p.valueInDollars > 50)
            [items addObject:p];
    }
    
    return items;
}

+ (NSArray *) possessionsUnder50
{
    NSMutableArray *items = [[NSMutableArray alloc] init];
    
    for (Possession *p in defaultStore.allPossessions)
    {
        if (p.valueInDollars < 50)
            [items addObject:p];
    }
    
    return items;
}

Adding the following to ItemsViewController.m

- (NSInteger) tableView:(UITableView *)tableView numberOfRowsInSection:(NSInteger)section
{
    if (section == 0)
        return [[PossessionStore possessionsUnder50] count];
    else
        return [[PossessionStore possessionsOver50] count];
}

- (UITableViewCell *) tableView:(UITableView *)tableView cellForRowAtIndexPath:(NSIndexPath *)indexPath
{
    UITableViewCell *cell = [tableView dequeueReusableCellWithIdentifier:@"UITableViewCell"];
    
    if (!cell)
        cell = [[[UITableViewCell alloc] initWithStyle:UITableViewCellStyleDefault
                                                    reuseIdentifier:@"UITableViewcell"] autorelease];
        
    if ([indexPath section] == 0)
        [[cell textLabel] setText:[[[PossessionStore possessionsOver50] objectAtIndex:[indexPath row]] description]];
    else //Must be section 1
         [[cell textLabel] setText:[[[PossessionStore possessionsUnder50] objectAtIndex:[indexPath row]] description]];
    
    return cell;
}

- (NSInteger)numberOfSectionsInTableView:(UITableView *)tableView
{
    return 2;
}

#2

Hi,

Here’s my feedback FWIW

You have

if (section == 0) // under 50 return [[PossessionStore possessionsUnder50] count];

And later

if ([indexPath section] == 0) //But now it’s Over50 [[cell textLabel] setText:[[[PossessionStore possessionsOver50] objectAtIndex:[indexPath row]] description]];

Easily done and I find that using #define helps avoid this

[code]#define POSSESSIONS_UNDER_50 0

if ([indexPath section] == POSSESSIONS_UNDER_50)
    [[cell textLabel] setText:[[possessionsUnder50 objectAtIndex:[indexPath row]] description]];

[/code]

Also you wont be reusing the tableView cells due to a typo

[code] UITableViewCell *cell = [tableView dequeueReusableCellWithIdentifier:@“UITableViewCell”];

if (!cell)
    cell = [[[UITableViewCell alloc] initWithStyle:UITableViewCellStyleDefault
                                                reuseIdentifier:@"UITableViewcell"] autorelease];

[/code]

If you make the identifier a variable then the compiler will catch a typo

[code] static NSString *cellID = @“UITableViewCell”;

UITableViewCell *cell = [tableView dequeueReusableCellWithIdentifier:cellID];

[/code]

Regarding the overall design:

Your class methods possessionsOver50 and possessionsUnder50 are going to be invoked every time that they are referred to. So the loop through defaultStore.allPossessions will be repeated for each row in the cellForRowAtIndexPath method and also the numberOfRowsInSection method. So thats 10 times per row and twice for the numberOfRowsInSection which means 120 passes through the array. When cells scroll off the screen the cellForRowAtIndexPath will be invoked again causing more iterations

Also the *items init/alloc isn’t ever released so this will be leaking memory each time they are called.

To stick with your general approach but make it more efficient I would make the following changes:

Make the under50 and over50 arrays instance variables:

[code]@interface PossessionStore : NSObject
{
NSMutableArray *allPossessions;
NSMutableArray *possessionsOver50;
NSMutableArray *possessionsUnder50;
}

  • (PossessionStore *) defaultStore;
  • (NSArray *) allPossessions;
  • (Possession *) createPossession;
  • (NSArray *) possessionsOver50;
  • (NSArray *) possessionsUnder50;

@end
[/code]

Initialise them once in the init method. Technically they are still leaking as we don’t explicitly release them but they have the lifetime of the app so we don’t need to worry.

[code]- (id)init
{
// If we already have an instance of PossessionStore…
if (defaultStore) {

    // Return the old one
    return defaultStore;
}

self = [super init];
if (self) {
    allPossessions = [[NSMutableArray alloc] init];
    possessionsOver50 = [[NSMutableArray alloc] init];
    possessionsUnder50 = [[NSMutableArray alloc] init];
}
return self;

}
[/code]

Create getters

[code]- (NSArray *) possessionsOver50
{
return possessionsOver50;
}

  • (NSArray *) possessionsUnder50
    {
    return possessionsUnder50;
    }
    [/code]

Populate them as each possession is created:

[code]- (Possession *)createPossession
{
Possession *p = [Possession randomPossession];

[allPossessions addObject:p];

if (p.valueInDollars < 50)
    [possessionsUnder50 addObject:p];
else
    [possessionsOver50 addObject:p];

return p;

}
[/code]

Change ItemsViewController to reflect the fact they are now instance variables

- (NSInteger) tableView:(UITableView *)tableView numberOfRowsInSection:(NSInteger)section { if (section == 0) return [[[PossessionStore defaultStore] possessionsUnder50] count]; else return [[[PossessionStore defaultStore] possessionsOver50] count]; }

And do the same in cellForRowAtIndexPath

I think that’s it - Good luck

Gareth


#3

Thanks for taking the time to respond. This is exactly what I was looking for.