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

Dev angel coronado quintero final delivery #105

Open
wants to merge 15 commits into
base: main-Angel-Coronado-Quintero
Choose a base branch
from

Conversation

AngelitoWizeline
Copy link
Collaborator

What does this PR do?

Movie Detail screen with MVVM architecture.

Where should the reviewer start?

Movie Detail

Code review checklist Code Review Rubric

  1. DRY Principle (Don’t repeat yourself)

    • Excellent
    • Good
    • Adequate
    • Developing
  2. Yagni Principle (You Aren't Gonna Need It)

    • Excellent
    • Good
    • Adequate
    • Developing
  3. KISS Principle (Keep It Simple, St...d)

    • Excellent
    • Good
    • Adequate
    • Developing
  4. Remove unused code

    • Excellent
    • Good
    • Adequate
    • Developing
  5. Hard-coded values

    • Excellent
    • Good
    • Adequate
    • Developing
  6. Language style guide & coding conventions

  1. Architecture / Design Pattern

    • MVC
    • MVP
    • MVVM
    • VIPER
  2. Architecture Implementation

    • Excellent
    • Good
    • Adequate
    • Developing
  3. SOLID

  • Single Responsibility Principle

    • Excellent
    • Good
    • Adequate
    • Developing
  • Open Closed Principle

    • Excellent
    • Good
    • Adequate
    • Developing
  • Liskov substitutability principle

    • Excellent
    • Good
    • Adequate
    • Developing
  • Interface segregation

    • Excellent
    • Good
    • Adequate
    • Developing
  • Dependency Inversion principle

    • Excellent
    • Good
    • Adequate
    • Developing
  1. Testing

    • Excellent
    • Good
    • Adequate
    • Developing
  2. Force unwrapping !

    • Excellent
    • Good
    • Adequate
    • Developing
  3. Mutable vs immutable property (Right use of let and var)

    • Excellent
    • Good
    • Adequate
    • Developing
  4. Retain cycles (Capture list)

    • Excellent
    • Good
    • Adequate
    • Developing
  5. Code Documentation

    • Excellent
    • Good
    • Adequate
    • Developing


import UIKit

class MovieDetailViewControllerMVVM: UIViewController, MovieDetailViewProtocol {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This should be final

import UIKit

class MovieDetailViewControllerMVVM: UIViewController, MovieDetailViewProtocol {
var viewModel: MovieDetailViewModelProtocol?
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This should be private

viewModel?.fetchAllDetailData()
}

func bind() {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This should be private

Comment on lines +48 to +56
self.title = viewModel?.getScreenTitle()
collectionView.collectionViewLayout = CompotitionalLayoutCreator.createLayoutForMovieDetail()
collectionView.setup(dataSource: self)
collectionView.setup(delegate: self)
collectionView.registerNibForCellWith(name: HeaderCollectionViewCell.identifierToDeque)
collectionView.registerNibForCellWith(name: MovieCollectionViewCell.identifierToDeque)
collectionView.reloadData()
let navigationItemReviews = UIBarButtonItem(image: UIImage(named: "reviews"), style: .plain, target: self, action: #selector(showReviewScreen))
self.navigationItem.rightBarButtonItem = navigationItemReviews
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You can make some functions with this


}

@objc func showReviewScreen() {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This should be private

}

@objc func showReviewScreen() {
print("show reviews")
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This should be deleted


extension MovieDetailViewControllerMVVM: UICollectionViewDataSource {
func numberOfSections(in collectionView: UICollectionView) -> Int {
return 2
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Try to avoid magic numbers

Comment on lines +11 to +33
protocol MovieDetailViewModelProtocol {
var movie: MovieProtocol { get set }
var movieDetail: MovieDetailProtocol? { get set }
var similarMovies: MovieList? { get set }
var apiDataManager: MoviesDetailAPIDataManagerProtocol? { get set }
func fetchDetailWith(movie: MovieProtocol)
var didFetchMovieDetail: ((_ movieDetal: MovieDetailProtocol) -> Void )? { get set }
var didFailFetchingMovieDetail: ((_ error: Error) -> Void)? { get set }
func fetchSimiliarMoviesWith(movie: MovieProtocol)
var didFetchSimilarMovies: ((_ movies: MovieList) -> Void)? { get set }
var didFailFetchingSimilarMovies: ((_ error: Error) -> Void)? { get set }
func fetchAllDetailData()
func fetchDetail()
func fetchSimiliar()
var router: MoviesDetailRouterProtocol? { get set }
func showMovieDetailWith(movie: MovieProtocol, from view: UIViewController)
func showMovieReviews(view: UIViewController)
func getScreenTitle() -> String
}

protocol MovieDetailViewProtocol {
var viewModel: MovieDetailViewModelProtocol? { get set }
}
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You can simplify this

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants