ViewModel::Auction

[harrisj]

I decided to make a ViewModel::Auction class to extract some common functionality from the ViewModel::AuctionIndex and ViewModel::AuctionShow methods as well as wrapping user-specific methods from Presenter::Auction

[baccigalupi]

Definitely an improvement, not having to pass along the current user.

[harrisj]

Yeah, I really wanted to get rid of those parameters

[adelevie]

looks good to me!

[baccigalupi]

I would use this oppty to wrap this into one method on the view model. Also, probabyl the current_user check isn't necessary. Would hope it is inside the #user_is_bidder? method

[harrisj]

I think I want to look at cleaning up the logic in this partial in a future revision because this is kinda ugly, right?

[baccigalupi]

Seems like an improvement to me. Got things in the view a little cleaner.

I am just a little worried that when all the if-else statements in the view are taken out, and when all the if-else-rescue statements are flattened in various presenters and view models, this will be the wrong place for logic, and its separation will make that harder to see.

[harrisj]

So, I do have a few questions I was wondering if you all could help me with:

1. Do you generally feel it's necessary to create tests for low-level simple classes like ViewModel::UserBids if I already have a test for something like ViewModel::Auction#user_bids that delegates to the lower class. I have 100% coverage, I guess my question is where the tests should go.
2. Should I be calling these ViewModel::Auction or ViewModel::UserBids or is there something else I should name them? Bear in mind that we might add something like ViewModel::AuctionListItem or such for components within the view HTML.
3. I do indeed have the concern that having everything spread out in a bunch of places will make it harder to see things, and I feel like some inline documentation of what these classes are meant to do will be helpful in the meantime.
4. I am still considering some way of replacing the SingleBid?/MultiBid? if-elses with polymorphism, but I still need to figure that out.
5. PlaceBid has some duplicated logic for figuring out things like the lowest bid and such. Should it be calling this ViewModel::Auction object instead or is there a good case for its logic to be self-contained?

[baccigalupi]

@harrisj about your questions:

1. I prefer testing at the lowest level possible. So the little leaf classes are better than the integration place. Don't test configuration and don't test how framework code works. So you want to test wherever you see branching logic, or non-trivial presentation changes in these classes.
2. ViewModels are everything that is needed for a given template. So, if there is a template that your are aggregating logic for, there is your name. If not then you are dealing with a presenter or something else. Name spacing is much less important than the right abstractions, although naming these things view models when they are not, will definitely confuse.
3. In future refactors, I would prioritize cleaning up the views, removing logic. Without knowing what logic is there, you are going to be guessing at where things should go and how to organize it. Documentation is a strong coding smell.
4. I would hold off for the right abstractions ... look up to point 3.
5. I would add a lot more duplication right now. Make it explicit. Copy and paste. Undry until you have a view model for each top level view. After that your can take things apart in a meaningful way. So, I would not at this point extract anything. I would collapse it all to look for real duplications.

[harrisj]

Thanks! With regards to point #2, ViewModel::Auction is not really tied to a specific view (like ViewModel::AuctionIndex is, but I wanted to remove some of the methods within the existing Presenter::Auction anyway. So, that's how we got here. Maybe it should be instead Presenter::UserAuction or something, but that's how it is now.

I should add tests for the BidsUser classes, but that might be a future PR.