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

Homework Completed #27

Open
wants to merge 13 commits into
base: master
Choose a base branch
from
Open

Homework Completed #27

wants to merge 13 commits into from

Conversation

cdolm92
Copy link

@cdolm92 cdolm92 commented Aug 15, 2015

No description provided.

@cdolm92
Copy link
Author

cdolm92 commented Aug 15, 2015

Still a work in progress...

@megantaylor
Copy link

IT WORKS! Congrats!

PastryCategories, PastriesTableView Controller, POTableViewController are not consistent with obj-c best practice
Categories are a thing, its look like you're trying to model data in to an object and they already exist (just name it pastry!)

Parent table view (types of pastries) owns 'category object' that gets passed around, makes it confusing as to who owns what information and how it is shared throughout the application. make it more clear how data moves around.
Using delegate pattern to reload the view instead of the correct way to pass info.

cell.backgroundColor = indexPath.row % 2
? [UIColor colorWithRed:(253.0/255.0) green:(235.0/255.0) blue:(247.0/255.0) alpha:1.0]
: [UIColor whiteColor];
ternary operators should be kept as minimal and obvious as possible
a 3-line ternary is not obvious

#pragma Navigation should be #pragma mark Navigation

@Property (nonatomic,weak) PastriesTableViewController *delegate; -> good!

@cdolm92 cdolm92 changed the title Some data Homework Completed Aug 18, 2015
@cdolm92
Copy link
Author

cdolm92 commented Aug 18, 2015

Hi Megan thanks for the reviewing my code and for providing helpful feedback. I will be implementing your suggestions in the next commits and keep these things in mind going forward.

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