Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

still working on it #30

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

still working on it #30

wants to merge 1 commit into from

Conversation

arturlan
Copy link

Mike, I haven't finished my homework, I'll try to finish it tomorrow.

- (void)viewDidLoad {
[super viewDidLoad];

self.category = @[@"Dogs", @"Cars", @"Food"];

Choose a reason for hiding this comment

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

Don't you want to these to be instances of CQCategory, not NSString?

Also, when naming variables/properties that hold collection data types like arrays, it's usually preferable to use the plural. In this case, categories is a better name than category, which implies that it references a single category, not all of them.

@tannerwelsh
Copy link

@arturlan I know you haven't finished yet, so when you do feel free to ping me and I can give more feedback. (By "ping" I mean leave a comment in this thread that @-mentions me.)

You're off to a good start, although there is one critical area of improvement that I want to point out. When you put closely related data (in this case, the category name and the category options) in separate places (in this case, one in each of your view controllers), it forces you to write some very dangerous and brittle code.

Specifically, in your viewDidLoad method of the DetailTableViewController, you've hard-coded a set of conditionals that link the array of options to the category by relying on the order of the categories in CategoryTableViewController. Now, every time you need to change the contents of your category property in CategoryTableViewController, you also need to remember to change your DetailTableViewController or else your code will break. This problem will only expand as your code grows.

A better way to structure this is to encapsulate closely related data by putting both the category name and its options all within a single object (i.e. CQCategory) instead of letting the float around in two separate arrays. That way, you know that whenever you are working with a particular category, you will also have access to its options (and vice versa). There is no need to check for its index within an array, or introduce other dependencies on how the categories are being stored. The important piece of data to pass around is the category object itself.

To change your code, you'll need to think about initializing all of your category objects in one place, and then passing them around between your view controllers as they need them.

For example, your DetailTableViewController might have a category property of type CQCategory that you set to the selected category in the prepareForSegue method of CategoryTableViewController, effectively sharing that object between those two controllers. That way, when DetailTableViewController loads, it can simply use the options property of its own category object to populate the table.

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.

2 participants