patternMinor
MasterViewController
Viewed 0 times
masterviewcontrollerstackoverflowprogramming
Problem
I plan on including this work in a portfolio.
Will this code get me hired or laughed at?
More specifically:
Follow-up questions:
-
Can you expand on what you said about magic numbers? (Totally missed the bool/BOOL usually I'm better than that. The names w/2 are just temp renames for SO)
-
I have a lot of .h files because I tried to refactor somethings, like animation and URL methods, into helpers, presumably to increase readability. Did I go overboard?
-
In theory, the way the program is setup, the MasterVC should never be deallocated. VC's are just pushed on top, not more than one layer at a time. Is it still necessary to unsubscribe from notifications?
-
Lastly, on a scale of 1-10 what is your impression of the code overall? (from what you've seen and discounting the name and bool problem)
Details:
MasterViewController.h
MasterViewController.m
```
#import "SRMasterViewController2.h"
#import "UIScrollView+SVPullToRefresh.h"
#import "UIScrollView+SVInfiniteScrolling.h"
#import "SRUrlHelper.h"
#import "SRNavBarHelper.h"
@interface SRMaster
Will this code get me hired or laughed at?
More specifically:
- How would you rate the general complexity of the code?
- How bad does the code smell?
- Is the VC too fat?
- Glaring best practices mistakes?
Follow-up questions:
-
Can you expand on what you said about magic numbers? (Totally missed the bool/BOOL usually I'm better than that. The names w/2 are just temp renames for SO)
-
I have a lot of .h files because I tried to refactor somethings, like animation and URL methods, into helpers, presumably to increase readability. Did I go overboard?
-
In theory, the way the program is setup, the MasterVC should never be deallocated. VC's are just pushed on top, not more than one layer at a time. Is it still necessary to unsubscribe from notifications?
-
Lastly, on a scale of 1-10 what is your impression of the code overall? (from what you've seen and discounting the name and bool problem)
Details:
- iOS: 6 (updating to 7 currently)
- Xcode: 4.6
- Tested: iPhone 4 device
- ARC Enabled
MasterViewController.h
#import
#import "SRAPI.h"
#import "SRChoiceBox.h"
#import "SRPostTopic.h"
#import "SRDetailViewController.h"
#import "SRCollapsibleCell.h"
#import "SRAnimationHelper.h"
#import "SROpenTokVideoHandler.h"
#import "SRObserveViewController.h"
@interface SRMasterViewController2 : UIViewController
@property (weak, nonatomic) IBOutlet UITableView *topicsTableView;
@property (weak, nonatomic) IBOutlet UIView *postTopicContainer;
@property (weak, nonatomic) IBOutlet UILabel *statusLabel;
@property (strong, nonatomic) NSIndexPath *openCellIndex;
@property (strong, nonatomic) RKPaginator *paginator;
@property (strong, nonatomic) SROpenTokVideoHandler *openTokHandler;
@endMasterViewController.m
```
#import "SRMasterViewController2.h"
#import "UIScrollView+SVPullToRefresh.h"
#import "UIScrollView+SVInfiniteScrolling.h"
#import "SRUrlHelper.h"
#import "SRNavBarHelper.h"
@interface SRMaster
Solution
A few things I would ask if I saw this as an interviewer (from a very quick read, and obviously not having seen it run):
Follow-up responses:
The real test though would be how well you can explain it during the interview, e.g. if you sound like its just a cut and paste job then the impression is lessened, but if you can explain your choices, pitfalls of the approach, anything you tried which you discard and why, and are able to talk about what you have learnt and where you would make improvements next time then you'd be looking pretty good I think.
- why use a UIViewController and not a UITableViewController (refresh control property is free then)
- the rasterisation on table view shouldn't be needed for smooth scrolling, you have other problems if that is required
- there are a few things which I wouldn't like to see and would ask about: magic numbers, names with a 2 at the end, inconsistencies (bool vs BOOL etc)
- why all the imports in the .h?
- why aren't the notification observers removed?
Follow-up responses:
- makes sense
- was just more about practice of importing .h files in a .h (meaning any class that needs to import your class knows about all those imported classes)
- It's just good practice I guess, but I didn't realise this was a permanent view controller
- Overall impressions are good (would depend a bit on the level of developer the job is for): the code is reasonably complex, uses 3rd party libraries, has some fairly advanced functions like dealing with layers / transforms.
The real test though would be how well you can explain it during the interview, e.g. if you sound like its just a cut and paste job then the impression is lessened, but if you can explain your choices, pitfalls of the approach, anything you tried which you discard and why, and are able to talk about what you have learnt and where you would make improvements next time then you'd be looking pretty good I think.
Context
StackExchange Code Review Q#33247, answer score: 4
Revisions (0)
No revisions yet.