hyperoslo / iOS-playbook

Hyper's iOS playbook
Other
227 stars 28 forks source link

ViewModel #112

Closed 3lvis closed 9 years ago

3lvis commented 9 years ago

So, ViewModel. I'm not sure if it's a "good practice" that I would like to follow, for me it's pretty cumbersome, for example just to get the attributed title of my Note I have to create a new instance. I'll rather have a category or extension containing this methods.

Prefered

// Here attributedTitle is contained in an extension of Note.
// This extension has the same principles of ViewModel
// without been obtrusive and cumbersome to use.

let note = Note(title: "Hi there")
self.viewController.title = note.attributedTitle

Not Prefered

let note = Note(title: "Hi there")
let noteViewModel = NoteViewModel(note)
self.viewController.title = noteViewModel.attributedTitle

What do you guys think?

@hyperoslo/ios

kostiakoval commented 9 years ago

I disagree. MVC is to separate model from the view and it kinda works now if you don't pass note object to the view (don't do that)

in your current example Note is also responsible for data representation (UI part, how it will be shown), it's not Single responsible anymore.

If another view would like also represent note but in other way, have other attributed string, where would you put it? in new note category? ViewModel should also should take care of data fetching and other Business logic. This way you move logic out of view controls and you can easily test it (because it's very very separated)

3lvis commented 9 years ago

If another view would like also represent note but in other way, have other attributed string, where would you put it? in new note category? ViewModel should also should take care of data fetching and other Business logic.

Yes, you could represent that in a bunch of categories/extensions too, I think the core idea of ViewModel is separation, not that you should make a separate class for that.

We should follow best practices that add to readability and reliability. I don't see how the second example (Not Preferred) is more readable, nor reliable than the first one.

kostiakoval commented 9 years ago

this is just one line of code, how about this one -

- (CGFloat)tableView:(UITableView *)tableView heightForRowAtIndexPath:(NSIndexPath *)indexPath
{
    CGFloat height = 230;

    if (FirstCell) {
        switch (indexPath.row) {
            case RankingHeaderCellRowIndex:
            case OrganizationalUnitHeaderCellRowIndex:
                height = TallerCellHeight;
                break;
            default:
                break;
        }
    } else {
        switch (indexPath.row) {
            case StoreCellRowIndex:
                height = TallerCellHeight;
                break;
            default:
                break;
        }
    }
    return height;
}

vs -

- (CGFloat)tableView:(UITableView *)tableView heightForRowAtIndexPath:(NSIndexPath *)indexPath
{
  return viewModel.heightForRowAtIndexPath(IndexPath)
}

Benefits:

kostiakoval commented 9 years ago

Business logic shouldn't be in ViewController. ViewController is responsible for data and even transferring between Model and View

3lvis commented 9 years ago

Hehehe, there's a misunderstanding here. I didn't meant to keep this helpers on the controller. I meant to keep them in the model in a different category/extension instead of a separate class called ViewModel.

3lvis commented 9 years ago

Haven't mentioned ViewController at all before.

kostiakoval commented 9 years ago

I know, but then you model class become a fat monster. and it would look strange to ask model do calculate size for table, or ask for how many cells it has

Example - tablebleView with notes, show will be responsible for calculating numbers of cells and rows and all other information? category on Note object ?

zenangst commented 9 years ago

@3lvis could you supply some more examples to avoid further confusion? :eyes:

vadymmarkov commented 9 years ago

Agree with @kostiakoval. Category / extension is a place to add some extra functionality, but in that case I don't think we want to extend our model class with view-related logic. And then call something like:

model.heightForRowAtIndexPath(IndexPath)
3lvis commented 9 years ago

First of all thanks for taking the time on sharing your thoughts, you guys are amazing.


Category / extension is a place to add some extra functionality. — @markvaldy

I don't think there are rules on what you should put on a category/extension. Please correct me if I'm wrong.


For me having a whole new class for this seems like an overkill and adding complexity just for the sake of following a pattern.

I don't see how this

// NoteViewModel.h
@interface NoteViewModel : NSObject
- (CGFloat)heightForRowAtIndexPath:(NSIndexPath *)indexPath;
@end
// ViewController.h
- (CGFloat)tableView:(UITableView *)tableView heightForRowAtIndexPath:(NSIndexPath *)indexPath
{
    Note *note = [self.dataSource objectAtIndexPath:indexPath];
    NoteViewModel *viewModel = [[NoteViewModel alloc] initWithNote:Note];
    return viewModel.heightForRowAtIndexPath(IndexPath)
}

Would be better, cleaner, more understandable and reliable than this

// Note+UITableView.h
@interface Note (UITableView)
- (CGFloat)heightForRowAtIndexPath:(NSIndexPath *)indexPath;
@end
// ViewController.h
- (CGFloat)tableView:(UITableView *)tableView heightForRowAtIndexPath:(NSIndexPath *)indexPath
{
    Note *note = [self.dataSource objectAtIndexPath:indexPath];
    return note.heightForRowAtIndexPath(IndexPath)
}

kostiakoval commented 9 years ago

self.dataSource should also got to ViewModel, you create viewModel in the viewDidLoad and use it all the time, you don't create it every time.

kostiakoval commented 9 years ago

How would you implement this method ?

- (NSInteger)tableView:(UITableView *)tableView numberOfRowsInSection:(NSInteger)section
{
    if ([self isStore]) {
        return 10;
    } else {
        return 20;
    }
}

vs

- (NSInteger)tableView:(UITableView *)tableView numberOfRowsInSection:(NSInteger)section
{
   return [self.viewModel numberOfRowsInSection:section]
}

All the logic goes to ViewModel.

3lvis commented 9 years ago

@kostiakoval Interesting points. It makes more sense when you think on detail views. Like having an app with a list of groceries then on the groceries list you wouldn't have a global viewModel because you don't have a global model on what to base your UI assumptions.

So you would have to create one ViewModel per cell.

Meanwhile on a the detail view controller, where you're only looking at one grocery then you'll create one global viewModel.

:)

ViewModel should also should take care of data fetching and other Business logic.

Is this correct though? Maybe we should constrain ViewModel to UI related code.

kostiakoval commented 9 years ago

You move stuff to datasource, why don't move more stuff out of viewController. Model is a data, is shouldn't anything about UI representation. It's for sure. @3lvis

3lvis commented 9 years ago

You move stuff to datasource, why don't move more stuff out of viewController.

What do you mean?

I consider the dataSource to be UI related since it's connected to usually to a UITableView or a UICollectionView.

But networking helpers, parsing helpers or business rules (like a BOOL to check if a contract has been signed) for example shouldn't go in a ViewModel.

kostiakoval commented 9 years ago

you wouldn't have a global viewModel because you don't have a global model on what to base your UI assumptions.

I not quite understand that Example: Groceries List App - GroceriesListViewMode - (init with list of groceries, or DATAStack) GroceryViewMode - (init with one item) GroceryDetailsViewMode - (init with one item)

GroceriesListViewMode responsibility:

kostiakoval commented 9 years ago

dataSource doesn't know how data will be used, will it displayed on UI or recover will do smt else with it. And it's great.

business rules (like a BOOL to check if a contract has been signed) for example shouldn't go in a ViewModel.

It should. This is exactly what ViewModels does. Example - Login form. Show should validate that email and password was entered? ViewModel, because it's about View and Logic

3lvis commented 9 years ago

Didn't we agreed on having a Fetcher? How would that fit here?

Also I updated my comment so it's more clear :)

kostiakoval commented 9 years ago

fetcher is only responsible for fetching data, I think example with form validation is really good. You would create some Validator. This is a Business logic, is should go out of VC

3lvis commented 9 years ago

I agree with the overall thought that doing ViewModel could improve our process. :)

Still don't quite agree on what should be the responsibilities but I guess that can be a separate discussion.

:two_men_holding_hands:

kostiakoval commented 9 years ago

:)

vadymmarkov commented 9 years ago

ViewModel is a really good approach, the example with Groceries List App from @kostiakoval reminds me MVVM from Windows Phone development, but + ViewController. The cool thing there is bindings which could be solved in some way by ReactiveCocoa in iOS.

kostiakoval commented 9 years ago

and if you don't live ReactiveCocoa you don't need to use it, you still have huge win splitting code into ViewModel

3lvis commented 9 years ago

React Components guys! It's the future!

The idea of it, don't focus on the framework or the language.