-2
\$\begingroup\$

I am new to iOS development. I wrote this code, but the company said that this is bad architecture and is not acceptable. I want to improve this code.

UITabBarController * tab = (UITabBarController *) self.window.rootViewController; 

UINavigationController * nav = (UINavigationController *) [[tab viewControllers] objectAtIndex: 0]; 

HSNEmployeesTableViewController * etc = (HSNEmployeesTableViewController *) [[nav viewControllers] 
objectAtIndex: 0]; 

etc.managedObjectContext = self.managedObjectContext;
\$\endgroup\$
  • \$\begingroup\$ This looks like code from somewhere in the controller layer of an app (probably a view controller, app delegate, or similar) but it's hard to be sure. In addition I think a major problem is how this component is obtaining the etc controller it is working with however it's hard to demonstrate what a better implementation might look like without any context. Can you provide a more complete example of what class this is in and what it is responsible for? \$\endgroup\$ – Jonah Aug 13 '14 at 5:09
  • \$\begingroup\$ The given code is in App Delegate file. It's like I have tab view controller with three tabs, first tab is like table view embed in navigation controller. The app is similar to contacts & implemented using core data. I know 1 more implementation of code it's look like: HSNEmployeesTableViewController * etc = (HSNEmployeesTableViewController *) nav.topViewController; I am not sure it's effective or it's will be good architecture for this. \$\endgroup\$ – Hitesh Nalamwar Aug 13 '14 at 6:11
  • 2
    \$\begingroup\$ I will comment on this later (after work), but between now and then, I'd like it if you could give some background on what parts of your app need to know about managedObjectContext as well as how self.managedObjectContext is instantiated in the first place by the app delegate. \$\endgroup\$ – nhgrif Aug 13 '14 at 11:41
  • \$\begingroup\$ Please add more context to this question. It is not very helpful to anyone in its current state. \$\endgroup\$ – bazola Aug 15 '14 at 3:13
6
\$\begingroup\$

Jonah's answer really doesn't address the architectural issue with your code. Yes, he makes valid points about your presumptive casts, but honestly, if this is what your company is talking about when they say your code has "architecture issues", I'd be a little amazed.

The primary problem that sticks out to me is this:

etc.managedObjectContext = self.managedObjectContext;

I'm presuming that this code is not only in your app delegate, but it's also most likely in your didFinishLaunching method.

So the questions become, how and why is the app delegate responsible for instantiating the managedObjectContext in the first place? What view controllers outside the HSNEmployeesTableViewController need to know about this object?

Can we not just instantiate this object in HSNEmployeesTableViewController's viewDidLoad method?

Code organization is very important. It's particularly important when you're working for a company or a group of coders where multiple people may be working in the same project and same source files. Nothing should have a scope any larger than is absolutely necessary. And objects should only be asked to do what makes sense for that object to do.

UITabBarController is a tab bar controller. The primary function of the tab bar controller is to control the tab bar view and manage the view controllers in the different tabs.

UINavigationController is a navigation controller. The primary function of the navigation controller is to control the navigation bar view and manage its navigation stack.

UIApplicationDelegate is the application's delegate object. The primary function of this class is to manage application-level events. All the methods in this class relate to application-level events such as starting up, exiting, going to background, coming from background, responding to notifications while in background, etc. We should not put logic in the application delegate that is not pertinent on an application-level.

The didFinishLaunching method is used to make a final few initializations that absolutely have to happen before your first view controller can be loaded.

Loading the data you want to present in the first view controller simply doesn't fit in this category.

You don't even have to worry about the casts, because your app delegate simply shouldn't need a reference to this view controller--the view controller can grab this data for itself.


As a pure critique of the posted code and ignore architecture issues, I offer this:

We can check the type of classes via isKindOfClass:. Additionally, let's keep in mind that objectAtIndex: can throw an out of bounds exception, but we only seem interested in index 0 in both cases, so let's use firstObject, shall we?

UITabBarController *tvc;
if ([self.window.rootViewController isKindOfClass:[UITabBarController class]]) {
    tvc = self.window.rootViewController;
}

UINavigationController *nvc;
if (tvc) {
    nvc = [tvc.viewControllers firstObject];
}

HSNEmployeesTableViewController *vc;
if ([nvc isKindOfClass:[UINavigationController class]]) {
    vc = [nvc.viewControllers firstObject];
}

if ([vc isKindOfClass:[HSNEmployeesTableViewController class]]) {
    vc.managedObjectContext = self.managedObjectContext;
} else {
    // something went wrong, handle it
}

Note that we will enter the else no matter what step went wrong. If tvc or nvc ends up being nil or not the kind of object we expect, we still get to the final if else and it will return NO and enter the else.

\$\endgroup\$
  • \$\begingroup\$ That's a good point but not one I thought I could tackle based on the limited example provided. I don't think it is necessarily reasonable for a view controller to create a managed object context. It's not appropriate for the app delegate to create that context either but it might be reasonable for the app delegate to coordinate supplying the context to a controller. \$\endgroup\$ – Jonah Aug 13 '14 at 22:10
5
\$\begingroup\$

Given the code provided the thing that stands out to me as a potentially poor design is the way you are obtaining references to controllers. Let's walk through why that might be a problem and what at least one alternative would be.

(UITabBarController *) self.window.rootViewController;

Here you obtain the window's root view controller. All you really know is that this is some UIViewController instance (assuming it has been set at all and is not nil). By casting it to a tab bar controller ((UITabBarController *)) you instruct the compiler to assume this is an instance of UITabBarController. That assumption is never checked or enforced. If the root view controller is not actually a tab bar controller then this application will eventually crash when an unrecognized selector is sent to that instance.

Your example code has a chain of three such casts. Each of which is piling assumptions on top of more assumptions about how this application is structured. That will likely work for now but you have tightly coupled your app delegate to the controller structure of your application. Should you ever change these controllers you must also change your app delegate as well. In addition if you do change your controllers this class will still compile, there will be no warning that these assumptions exist until your app crashes.

The pattern you have now is unfortunately common when controllers are created in a storyboard. Apple's documentation gives examples of doing the same thing in the "Coordinating Efforts Between View Controllers" section of the "View Controller Programming Guide for iOS".

Let's step back and think about what this portion of the app delegate is responsible for. It seems to be trying to make sure a HSNEmployeesTableViewController is correctly initialized and able to use your Core Data stack. The app delegate therefore needs a reference to the HSNEmployeesTableViewController but it should not necessarily need to dig through the current controller tree to find it.

One option would be to leave the current implementation but at least add checks and assertions after each cast. That way the app will crash immediately (and hopefully with a useful assertion message) if you change the storyboard layout such that a statement like [[nav viewControllers] objectAtIndex: 0] no longer returns the type of object you expected.

Another option is to provide your controller dependencies (the managed object context in this case) to the root view controller and have each controller pass those dependencies on to their children as needed when performing a segue. Downsides to that include needing to subclass lots of controllers in order to implement segue lifecycle methods and requiring each controller to be aware of the union of all it's current children's dependencies.

Personally I prefer to use a dependency injection container (like the Objection library) to provide a mechanism for supplying the dependencies of an instance. Each custom controller subclass can then request its dependencies when it is instantiated. One of my coworkers wrote about this approach recently: http://blog.carbonfive.com/2014/07/09/use-objection-with-uiviewcontrollers-and-storyboards/

\$\endgroup\$
  • \$\begingroup\$ This doesn't really comment on the actual architecture. Perhaps the OP doesn't understand the architectural problems his company is talking about though. \$\endgroup\$ – nhgrif Aug 13 '14 at 19:59
-2
\$\begingroup\$

The code looks fine in terms of obtaining pointers to the objects. However there is no check for nil and the code is blindly using these pointers, thus either causing a crash or unexpected results. As a C developer this is what I see first. As for the other points the other posters recommend I cannot comment and I would recommend using their advice.

\$\endgroup\$
  • \$\begingroup\$ There is no crash when you call methods on nil in Objective-C. It's a language feature. The only potential crash is calling objectAtIndex: without first checking that the array would contain that index. \$\endgroup\$ – nhgrif Aug 19 '14 at 17:14

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.