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

Incorrect checking if destinationViewController has moduleInput method #7

Closed
Antowkos opened this issue Jan 13, 2016 · 8 comments
Closed

Comments

@Antowkos
Copy link

Look UIViewController+RamblerViperModuleTransitionHandlerProtocol.m at lines 112-115, there you can see the following:

id<RamblerViperModuleTransitionHandlerProtocol> targetModuleTransitionHandler = destinationViewController;
if ([targetModuleTransitionHandler respondsToSelector:@selector(moduleInput)]) {
    moduleInput = [targetModuleTransitionHandler moduleInput];
}

and lines 28-34:

- (id)moduleInput {
    id result = objc_getAssociatedObject(self, @selector(moduleInput));
    if (result == nil && [self respondsToSelector:@selector(output)]) {
        result = [(id<TranditionalViperViewWithOutput>)self output];
    }
    return result;
}

This checking is incorrect, because if you implement method in category of some object (for example UIViewController), like you do at lines 28-34, all objects of this class (all UIViewControllers) will respond to this method even if you won't import this category.

Also look UIViewController+RamblerViperModuleTransitionHandlerProtocol.h at line 11, there you can see the following:

@interface UIViewController (RamblerViperModuleTransitionHandlerProtocol)<RamblerViperModuleTransitionHandlerProtocol>

This mean that all objects of this class (all UIViewControllers) will conform to this protocol, so even if you will try to change old checking with something like this:

if ([destinationViewController conformsToProtocol:@protocol(RamblerViperModuleTransitionHandlerProtocol)]) {
    moduleInput = [destinationViewController moduleInput];
}

every UIViewController will pass this check.

Because of this, when i try to combine VIPER module with old MVC like this:

[[self.transitionHandler openModuleUsingSegue:@"..."] thenChainUsingBlock:^id<RamblerViperModuleOutput>(id<RamblerViperModuleInput> moduleInput) {
    return nil;
}];

i got nil in moduleInput, instead of expected segue.destinationViewController.

@AndreyZarembo
Copy link
Contributor

Hi!
This category was added for convince and it's intentionally makes any UIViewController to conform this checks. It helps us to avoid use of parent ViewController.

As i understand, you'd like to return some ViewController as it's module input. If so, you can just add moduleInput method with return of self to it.

- (id)moduleInput {
    return self;
}

@Antowkos
Copy link
Author

Thanks for your reply!

The solution is easy, but not obvious for those who just start to move from MVC. I think you should write about this case on main page, because you announce full support of old MVC pattern, but it isn't so.

@Antowkos
Copy link
Author

also using of instancetype is better than id for return.

- (instancetype)moduleInput {
    return self;
}

@AndreyZarembo
Copy link
Contributor

I'll add FAQ section with this question and some others.

Why do you think instancetype will be better?

@Antowkos
Copy link
Author

When you use id, you get essentially no type checking at all. With instancetype, the compiler and IDE know what type of thing is being returned, and can check your code better and autocomplete better.

@AndreyZarembo
Copy link
Contributor

id is expected type for return value of moduleInput method(getter of this property). And it's ok to return not only ViewController, but special object to handle moduleInput. This is the reason why id is used.
instancetype usage will limit types of moduleInput to only ViewController of such class and it's children. So none of child controllers could use special object, only itself. Such limitation is not needed in this case.

@Antowkos
Copy link
Author

Yes, instancetype will limit types and it's ok for case when i will add moduleInput method as getter to some ViewController i need.

Anyway if statement at line 113 is unnecessary.


And one more thing. I think lines 106-110 is little bit confusing:

UIViewController *destinationViewController = segue.destinationViewController;
if ([destinationViewController isKindOfClass:[UINavigationController class]]) {
  UINavigationController *navigationController = segue.destinationViewController;
  destinationViewController = navigationController.topViewController;
}

Of course, it's great if you will get moduleInput from navigationController but what if destinationViewController will be UITabBarController, for example, or some user-specific container like some side-menu controller? I will get this controllers as moduleInput and if i will try to call some configure methods the app will crash, because there are no such method.

@AndreyZarembo
Copy link
Contributor

Developer should know what module will be called. In case of custom container or tab bar you can return any child controller from moduleInput method to configure it. Moreover it's better to configure all child controllers via parent container in such case.

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

No branches or pull requests

2 participants