App crashes when scrolling on the sections challenge


#1

I am working on the challenge but having some issues. I have done all my sorting of the possessions in the ItemsViewController. I have the section headers working correctly and I am using 2 NSArray’s to sort the possessions via filteredArrayUsingPredicate:. I got that working right too. My problem is that the app crashes when you try to scroll the table. Through trial and error I have been able to pin it down to my tableView:cellForRowAtIndexPath: method. I stopped trying to put possessions in the cells and just a simple string and it works fine, but crashes every time you try to scroll if I add back in the possessions. Any help is appreciated.
Here’s what I have going.
ItemsViewController.h

@interface ItemsViewController : UITableViewController
{
	NSArray *cheapStuff;
	NSArray *expensiveStuff;
}
@end

Here is what I have in ItemsViewController.m

- (NSInteger)tableView:(UITableView *)tableView numberOfRowsInSection:(NSInteger)section
{
	if (section == 0) {
			return [cheapStuff count];
	}
	return [expensiveStuff count];
}

- (NSInteger) numberOfSectionsInTableView:(UITableView *)tableView
{
	NSPredicate *cheap = [NSPredicate predicateWithFormat:@"valueInDollars < 50"];
	NSPredicate *expensive = [NSPredicate predicateWithFormat:@"valueInDollars >= 50"];
	
	cheapStuff = [[[PossessionStore defaultStore] allPossessions] filteredArrayUsingPredicate:cheap];
	expensiveStuff = [[[PossessionStore defaultStore] allPossessions] filteredArrayUsingPredicate:expensive];
	
	//NSLog(@"The Cheap Stuff %@", cheapStuff);
	if ([cheapStuff count] > 0 && [expensiveStuff count] > 0) {
		return 2;
	}
		return 1;
}

- (NSString *) tableView:(UITableView *)tableView titleForHeaderInSection:(NSInteger)section
{
	if (section == 0) {
		return @"Cheap Stuff";
	}
	return @"Expensive Stuff";
}

- (UITableViewCell *)tableView:(UITableView *)tableView cellForRowAtIndexPath:(NSIndexPath *)indexPath
{
		// Check for reusable cell
	UITableViewCell *cell = [tableView dequeueReusableCellWithIdentifier:@"UITableViewCell"];
		// If there is no cell of this type create a new one
	if (!cell) {
		cell = [[[UITableViewCell alloc] initWithStyle:UITableViewCellStyleDefault reuseIdentifier:@"UITableViewCell"] autorelease];
	}
	Possession *p;
		//NSArray *store;
		//set the tect on the cell with the description of the possesion
		// that is at the index path corresponding to the table row
		//Possession *p = [[[PossessionStore defaultStore] allPossessions] objectAtIndex:[indexPath row]];
		//[[cell textLabel] setText:[p description]];
	if ([indexPath section]  == 0) {
		p = [cheapStuff objectAtIndex:[indexPath row]];
			//[[cell textLabel] setText:[NSString stringWithFormat:@"Section 1, Row %d", [indexPath row] + 1]];//[p description]];	
	} else if ([indexPath section]  == 1){
		p = [expensiveStuff objectAtIndex:[indexPath row]];
			//[[cell textLabel] setText:[NSString stringWithFormat:@"Section 2, Row %d", [indexPath row] + 1]];//
	}
	[[cell textLabel] setText:[p description]];
	return cell;
}

#2

Okay so I never did figure this out using this approach. I think it has something to do with the retain cycle of my to sub arrays but still not sure. Anyway after getting some sleep and rethinking the challenge over coffee I decided maybe the logic for what array should be returned to the table view should really be handled by the possessionStore based on a query of what the tableViewController wants. So I approached it from that angle. I am not sure about the elegance or efficiency I have implemented as I had to add extra support functions to the store and tableview controller to make it work but it does work smoothly. I’ve put it through the ringer and it doesn’t crash. I also tested the dynamic nature of the section counts and headers by spiking the valueInDollars amount in randomPossession:.

Here is what I have done for any interested. I welcome any feedback on this method or what would have made it work in my original attempt.
PossessionStore Changes:
PossessionStore.h

[code]@class Possession;

@interface PossessionStore : NSObject
{
NSMutableArray *allPossessions;
NSArray *stuff; // this is my new array for returning subsets of the data
}

  • (PossessionStore *)defaultStore;

-(NSArray *)allPossessions;
-(Possession *)createPossession;
-(NSArray *)possesionsFromPredicate:(NSString *)predicateString; // this function determines if a subset of data or all possessions get returned

@end[/code]
PossessionStore.m implementation of possessionsFromPredicate

[code]-(NSArray *)possesionsFromPredicate:(NSString *)predicateString
{
NSLog(@“Returning stuff from predicate:%@”, predicateString);

if (predicateString == @"all") {
	return allPossessions;
}
NSPredicate *predicate = [NSPredicate predicateWithFormat:predicateString];
stuff = [[self allPossessions] filteredArrayUsingPredicate:predicate];
return stuff;

}
[/code]
ItemsViewController still saw some heavy changes
ItemsViewController.h

@interface ItemsViewController : UITableViewController
{
	int sections; // variable to hold the number of sections
}
- (void)setNumberOfSections; // this function determines the number of sections needed
@end[/code]
Here is ItemsViewController.m
[code]@implementation ItemsViewController

- (id) init
{
		// Call the superclass's designated initializer
	self = [super initWithStyle:UITableViewStyleGrouped];
	if (self) {
		for (int i = 0; i < 100; i++) {
			[[PossessionStore defaultStore] createPossession];
		}
	}
	[self setNumberOfSections]; // set the number of sections on initialization
	return self;
}

- (id) initWithStyle:(UITableViewStyle)style
{
	return [self init];
}

- (NSInteger)tableView:(UITableView *)tableView numberOfRowsInSection:(NSInteger)section
{
	NSLog(@"numberOfRowsInSection: called for section %i", section);
	
		// determine the number of rows needed for section
	int numberOfItems;
	
		//1 section = count of all possesions
	if (sections == 1) {
		numberOfItems = [[[PossessionStore defaultStore] possesionsFromPredicate:@"all" ] count];
	}
	
		//If we have 2 sections count the array for the current section based on the query current section
	if (sections == 2 && section == 0) {
		numberOfItems = [[[PossessionStore defaultStore] possesionsFromPredicate:@"valueInDollars < 50" ] count];
	} else if (sections == 2 && section == 1) {
		numberOfItems = [[[PossessionStore defaultStore] possesionsFromPredicate:@"valueInDollars >= 50" ] count];
	}
	
		// return the number of items
	return numberOfItems;
}
-  (void)setNumberOfSections
{
		// Determine if we need more than 1 section and return the number of sections
	if ([[[PossessionStore defaultStore] possesionsFromPredicate:@"valueInDollars >= 50"] count] > 0 
		&& [[[PossessionStore defaultStore] possesionsFromPredicate:@"valueInDollars < 50" ] count] > 0) {
		sections = 2;
	} else {
		sections=1;
	}
}

- (NSInteger) numberOfSectionsInTableView:(UITableView *)tableView
{
	NSLog(@"returning number of sections: %i", sections);
		// return the value of sections variable
	return sections;
}

- (NSString *) tableView:(UITableView *)tableView titleForHeaderInSection:(NSInteger)section
{
		//Configure the header titles based on the number of sections
	if (sections == 1) {
		return @"Stuff";
	} else if (section == 0) {
		return @"Cheap Stuff";
	}
	return @"Expensive Stuff";
}

- (UITableViewCell *)tableView:(UITableView *)tableView cellForRowAtIndexPath:(NSIndexPath *)indexPath
{
		// Check for reusable cell
	UITableViewCell *cell = [tableView dequeueReusableCellWithIdentifier:@"UITableViewCell"];
		// If there is no cell of this type create a new one
	if (!cell) {
		cell = [[[UITableViewCell alloc] initWithStyle:UITableViewCellStyleDefault reuseIdentifier:@"UITableViewCell"] autorelease];
	}
	
		// configure predicate string based on the number of sections and current section
	NSString *predicateString;
		// Section 0 is the sub $50 section and 1 is the over $50 section
	if (sections > 1 && [indexPath section] == 0) {
		predicateString = @"valueInDollars < 50";
	} else if (sections > 1 && [indexPath section] == 1) {
		predicateString = @"valueInDollars >= 50";
	} else {
			// default to allPossesions for only 1 section
		predicateString = @"all";
	}
		//set the text on the cell with the description of the possesion
		// that is at the index path corresponding to the table row
	
		// Note the only change here is a call to possesionsFromPredicate:predicateString instead of allPossessions
	
	Possession *p = [[[PossessionStore defaultStore] possesionsFromPredicate:predicateString] objectAtIndex:[indexPath row]];
	[[cell textLabel] setText:[p description]];

	return cell;

}

@end

#3

Hi,

In the first example the crash is due to the cheapStuff and expensiveStuff arrays not being retained.
filteredArrayUsingPredicate returns a new array that is autoreleased so if you want it to hang around then you will need to retain it.
I would move the creation of these arrays to the init method and retain them - and don’t forget to release them in dealloc.

Your second example gets round this by recreating the arrays every time they are referenced (cellForRow, numberOfSections etc) so they are still available within the method that refers to them and hence no crashes.
The problem though is that this is very inefficient as cellForRow is called every time the tableView needs a cell (initialisation, scrolling, reloadData etc). You probably won’t notice with arrays around 100 rows but bump this up to 10,000 and things will get very sluggish.

In this example allPossessions doesn’t change after initialisation so you can safely create these arrays once, retain them, and refer to them wherever they are used.
If they can change in the future (You add editing capability for example and can change a cheap thing into an expensive thing) then they would need to be refreshed. But it would be better to do this when the edit occurs rather than every time they are displayed.

HTH
Gareth


#4

Hi Gareth, Thanks for the feedback. I had a feeling it was due to a release of the arrays. Despite a few trips through chapter 3, I am still trying to get the hang of memory management. Definitely been the hardest thing for me so far.

I also had a feeling recreating the sub arrays every time was pretty inefficient. I agree for this project your suggestions are the way to go and will implement them.

For my own future reference can you comment, with respect to design and developing principles, which is the best way to go? Letting the store decide what to return when needed, or is it best that the store stays dumb returning everything at once and let the tableViewController sort what it needs? Will the strategy change later on when only loading subsets of the data, and you have the infamous more… button in the last cell?

I have some CS under my belt, but I haven’t finished my degree. So far my school training has been in C++, and my instinct from training there was that the store should be an abstract type that returns only what the table requests from it at a given time. Is this just wrong all the way around, different in objective C practice, failed understanding on my part, or possibly bad instruction from professors? I do want to be the best dev I can be. Any thoughts on this are welcome from you or anyone else.


#5

Hi,

It’s very difficult to answer What is the best way? as it always depends on many, many factors.

You are always trying to balance available machine resources, responsiveness, functionality, code reusability, language features, maintainability etc etc and invariably there has to be a compromise or two made somewhere.

As a general principle I think it’s sensible not to retrieve and return data that isn’t going to be used and so returning everything from a store and letting the tableView filter it is wasteful and may lead to performance issues if the data grows. However there are occassions when it is neccessary to load everything into memory and filter as needed to get the responsiveness required.

Letting the store decide how much data to return has its own problems. It might work out fine initially but then later you decide to reuse the store in another project - how would the store know how much data is appropriate?

So I would suggest that you design the store to do what it is told. If you ask it for everything it should give you everything. If you ask for a subset then it should give you a subset. Each app/view can then make its own decisions about how much data is appropriate. It can also go beyond the number of rows required. Sometimes you need to limit the number of attributes returned as they are not displayed on the sum :smiley: mary screen and you only retrieve the full set when the user navigates to the detail screen.

So I would say your instinct (and your professors !) are right - the store should return what is requested of it.

The tableView is pretty smart at not wasting resources for display with its dequeueReusableCell feature and there are hundreds / thousands of apps where the datasource is an array, initialised on app startup or view load and filtered or not as needed. There is a sound principle of not prematurely optimising in software development and so often this will be the right approach. However if the dataset is very large or is provided via a network then you may be forced to refine the retrieval mechanism to get any kind of useable performance.

If it’s any consolation, my path through the book was Chapter 1,2,3,4,3,5,3,6,3,7,3,8,3… :smiley:

Gareth


#6

While not very reassuring about the future, it is comforting to see I’m not the only one with an issue on that subject.

So it kind of sounds to me that the best balance in this case is that the store does in fact return only what is needed from it so most of my changes to PossessionStore should stay. The needed change would be in the frequency the tableView pulls changes from it??? :unamused: Instead of pulling during cellForRow… ,I should have my section arrays created in itemsViewController during init and updated again whenever the table data is modified??? :unamused:

With an eye for future flexibility i could try some thing like this :unamused:

give itemsViewController 2 arrays
NSMutableArray *sections;
NSMutableArray *sectionsHeaders;

initailize them in init

sections = [NSMutableArray alloc] init];
sectionsHeaders = [NSMutableArray alloc] init];
// now these arrays are retained along with anything I put in them until I release in dealloc: or viewDidUnload ??? :unamused:

[self dividePossessions];

now implement something like
-(void)dividePossessions:
{
// an if statement to determine if the sections are being modified or created the first time and handle accordingly,
// sections would be created like this though
[sections addObject:[[possesionStore defaultStore] possesionsFromPredicate:@“valueInDollars < 50”]
// call again for each desired section adding arrays to section.

Then of course I can set section count from the count in the sections array and name section headers in the sectionHeaders array with matching indexes.

then recall dividePossessions again anytime a possesion is modified, added or deleted.

in tableRowForSection…
you just get items from the sections arrays based on section number

I know much more detail would go into implementing this but i think i have simplified the idea of it. :unamused:

Thoughts???


#7

Hi,

Sounds good to me - you will have a very flexible design for adding further categories in the future.

Also, remember that when you add an object to a NSArray it bumps the retain count up by 1. So the autoreleased array that possesionsFromPredicate returns won’t need an additional retain in your new approach because you are adding it to the sections array.

You could also consider making sections an array of NSDictionary’s with one key for the section name and one for the associated array of qualifying possessions.

Good luck.

Gareth


#8

Thanks for the feedback and confirming my instincts on the sections array retaining it’s objects. I’m going to work on implementing this system and see how it goes. I’ll post my code for feedback after. Nice tip on the NSDictionary too. Not much experience with those yet. I’ll definitely look into it. :smiley: