home-view: update homeViewController #49
Open
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Kros-s
reviewed
Nov 18, 2021
| var viewMoreButton = UIButton() | ||
| var collectionView: UICollectionView! | ||
|
|
||
| let service = NetworkManager(urlSession: URLSession.shared) |
There was a problem hiding this comment.
make it private since it's a dependency and inject over init to allow dependency injection
Kros-s
reviewed
Nov 18, 2021
| let service = NetworkManager(urlSession: URLSession.shared) | ||
| static let categoryHeaderUpcoming = "Upcoming" |
Kros-s
reviewed
Nov 18, 2021
| var movies: [Movie] = [] { | ||
| didSet { | ||
| collectionView.reloadData() | ||
| } | ||
| } | ||
|
|
||
| init() { | ||
| super.init(collectionViewLayout: HomeViewController.createLayout()) |
There was a problem hiding this comment.
call a method instead of creating static or Factory pattern can be used here, this just to avoid exposing static elements
Kros-s
reviewed
Nov 18, 2021
| static func createLayout() -> UICollectionViewCompositionalLayout { | ||
| let item = NSCollectionLayoutItem(layoutSize: .init(widthDimension: .fractionalWidth(1), heightDimension: .fractionalHeight(1))) | ||
| return UICollectionViewCompositionalLayout { (sectionNumber, env) -> NSCollectionLayoutSection in | ||
| if sectionNumber == 0 { |
There was a problem hiding this comment.
No magic numbers, this is risky what if by any reason order change ? wouldn't it be better to use an enum or constant instead ?
Kros-s
reviewed
Nov 18, 2021
Comment on lines
+30
to
+46
| let group = NSCollectionLayoutGroup.horizontal(layoutSize: .init(widthDimension: .fractionalWidth(1), heightDimension: .absolute(200)), subitems: [item]) | ||
| item.contentInsets.trailing = 10 | ||
| item.contentInsets.leading = 10 | ||
| item.contentInsets.bottom = 16 | ||
| let section = NSCollectionLayoutSection(group: group) | ||
| section.orthogonalScrollingBehavior = .paging | ||
| return section | ||
| } else { | ||
| let item = NSCollectionLayoutItem(layoutSize: .init(widthDimension: .fractionalWidth(0.33), heightDimension: .absolute(150))) | ||
| item.contentInsets.trailing = 10 | ||
| item.contentInsets.bottom = 5 | ||
| let group = NSCollectionLayoutGroup.horizontal(layoutSize: .init(widthDimension: .fractionalWidth(1), heightDimension: .estimated(500)), subitems: [item]) | ||
|
|
||
| let section = NSCollectionLayoutSection(group: group) | ||
| section.contentInsets.leading = 10 | ||
| section.boundarySupplementaryItems = [ | ||
| .init(layoutSize: .init(widthDimension: .fractionalWidth(1), heightDimension: .absolute(50)), elementKind: categoryHeaderUpcoming, alignment: .topLeading) |
Kros-s
reviewed
Nov 18, 2021
Comment on lines
+60
to
+63
| collectionView.register(MovieCell.self, forCellWithReuseIdentifier: "cell") | ||
| collectionView.register(HeaderOfSection.self, forSupplementaryViewOfKind: HomeViewController.categoryHeaderUpcoming, withReuseIdentifier: categoryHeaderUpcomingId) | ||
| navigationItem.title = "Movies" | ||
|
|
There was a problem hiding this comment.
move this into a method called prepareLayout or setupLayout
Kros-s
reviewed
Nov 18, 2021
Comment on lines
+84
to
+86
| let cell = collectionView.dequeueReusableCell(withReuseIdentifier: "cell", for: indexPath) as! MovieCell | ||
| cell.label?.text = movies[indexPath.row].title | ||
| return cell |
There was a problem hiding this comment.
avoid forcewrapping use a guard instead
Kros-s
reviewed
Nov 18, 2021
| return cell | ||
| } | ||
| override func numberOfSections(in collectionView: UICollectionView) -> Int { | ||
| 2 |
There was a problem hiding this comment.
this is a magic number, we should know from something how many sections are present sorta like movies var
Kros-s
reviewed
Nov 18, 2021
| } | ||
| override func collectionView(_ collectionView: UICollectionView, viewForSupplementaryElementOfKind kind: String, at indexPath: IndexPath) -> UICollectionReusableView { | ||
| let header = collectionView.dequeueReusableSupplementaryView(ofKind: kind, withReuseIdentifier: categoryHeaderUpcomingId, for: indexPath) as! HeaderOfSection | ||
| header.label.text = "Upcoming" |
There was a problem hiding this comment.
why we create a static element on top and not using it here ?
Kros-s
reviewed
Nov 18, 2021
| // | ||
|
|
||
| import Foundation | ||
| class MakeRequest { |
Kros-s
reviewed
Nov 18, 2021
| self?.handleResponse(response) | ||
| } | ||
| } | ||
|
|
Kros-s
reviewed
Nov 18, 2021
| } | ||
| let service = NetworkManager(urlSession: URLSession.shared) | ||
|
|
||
|
|
Kros-s
reviewed
Nov 18, 2021
|
|
||
| import Foundation | ||
|
|
||
| class GetLatestMoviesRepositoryImpl: GetLatestMoviesRepository{ |
Kros-s
reviewed
Nov 18, 2021
|
|
||
| import Foundation | ||
|
|
||
| class GetLatestMoviesRepositoryImpl: GetLatestMoviesRepository{ |
There was a problem hiding this comment.
Suggested change
| class GetLatestMoviesRepositoryImpl: GetLatestMoviesRepository{ | |
| class GetLatestMoviesRepositoryImpl: GetLatestMoviesRepository { |
Kros-s
reviewed
Nov 18, 2021
| private let basePath: String = URLRequestType.popular.basePath | ||
| let service = NetworkManager(urlSession: URLSession.shared) | ||
|
|
||
| func handleResponse(_ response: MovieList) { |
There was a problem hiding this comment.
make it private
Suggested change
| func handleResponse(_ response: MovieList) { | |
| private func handleResponse(_ response: MovieList) { |
Kros-s
reviewed
Nov 18, 2021
|
|
||
| import Foundation | ||
|
|
||
| class GetNowPlayingMoviesRepositoryImpl: GetNowPlayingMoviesRepository { |
There was a problem hiding this comment.
fianl
Suggested change
| class GetNowPlayingMoviesRepositoryImpl: GetNowPlayingMoviesRepository { | |
| final class GetNowPlayingMoviesRepositoryImpl: GetNowPlayingMoviesRepository { |
Kros-s
reviewed
Nov 18, 2021
Comment on lines
+21
to
+22
|
|
||
| func handleResponse(_ response: MovieList) { |
There was a problem hiding this comment.
Suggested change
| func handleResponse(_ response: MovieList) { | |
| private func handleResponse(_ response: MovieList) { |
Kros-s
reviewed
Nov 18, 2021
|
|
||
| import Foundation | ||
|
|
||
| class GetPopularMoviesRepositoryImpl: GetPopularMoviesRepository { |
There was a problem hiding this comment.
Suggested change
| class GetPopularMoviesRepositoryImpl: GetPopularMoviesRepository { | |
| final class GetPopularMoviesRepositoryImpl: GetPopularMoviesRepository { |
Kros-s
reviewed
Nov 18, 2021
| class GetPopularMoviesRepositoryImpl: GetPopularMoviesRepository { | ||
| var movies: [Movie] = [] | ||
| private let basePath: String = URLRequestType.popular.basePath | ||
| let service = NetworkManager(urlSession: URLSession.shared) |
There was a problem hiding this comment.
Suggested change
| let service = NetworkManager(urlSession: URLSession.shared) | |
| let service = NetworkManager(urlSession: URLSession.shared) // No singleton should be called here directly |
Kros-s
reviewed
Nov 18, 2021
|
|
||
| import Foundation | ||
|
|
||
| class GetTopRatedMoviesRepositoryImpl: GetListMoviesRepository { |
There was a problem hiding this comment.
Suggested change
| class GetTopRatedMoviesRepositoryImpl: GetListMoviesRepository { | |
| final class GetTopRatedMoviesRepositoryImpl: GetListMoviesRepository { |
Kros-s
reviewed
Nov 18, 2021
| // | ||
|
|
||
| import Foundation | ||
| class GetUpcomingMoviesRepositoryImpl: GetUpcomingMoviesRepository { |
There was a problem hiding this comment.
Suggested change
| class GetUpcomingMoviesRepositoryImpl: GetUpcomingMoviesRepository { | |
| final class GetUpcomingMoviesRepositoryImpl: GetUpcomingMoviesRepository { |
Kros-s
reviewed
Nov 18, 2021
| } | ||
| } | ||
|
|
||
| func handleResponse(_ response: MovieList) { |
There was a problem hiding this comment.
Suggested change
| func handleResponse(_ response: MovieList) { | |
| private func handleResponse(_ response: MovieList) { |
Kros-s
reviewed
Nov 18, 2021
| // | ||
|
|
||
| import Foundation | ||
| class MoviesRepositoryImpl { |
There was a problem hiding this comment.
Suggested change
| class MoviesRepositoryImpl { | |
| final class MoviesRepositoryImpl { |
Kros-s
reviewed
Nov 18, 2021
|
|
||
| func requestMovies() { | ||
| service.get(path: basePath) { [weak self] response in | ||
| _ = self?.handleResponse(response) |
There was a problem hiding this comment.
this can bring a race condition since you are returning values
Kros-s
reviewed
Nov 18, 2021
Comment on lines
+26
to
+28
| movies = response.results | ||
| } | ||
| return movies |
Kros-s
reviewed
Nov 18, 2021
| import Foundation | ||
| import UIKit | ||
|
|
||
| class HeaderOfSection: UICollectionReusableView { |
Kros-s
reviewed
Nov 18, 2021
Comment on lines
+12
to
+14
| let label = UILabel() | ||
| var title: String? | ||
|
|
There was a problem hiding this comment.
do you really need this cell what about using a default one ?
Kros-s
reviewed
Nov 18, 2021
|
|
||
| override func layoutSubviews() { | ||
| super.layoutSubviews() | ||
| label.frame = bounds |
Kros-s
suggested changes
Nov 18, 2021
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
What does this PR do?
Where should the reviewer start?
Code review checklist Code Review Rubric
DRY Principle (Don’t repeat yourself)
Yagni Principle (You Aren't Gonna Need It)
KISS Principle (Keep It Simple, St...d)
Remove unused code
Hard-coded values
Language style guide & coding conventions
Wizeline Style Guide
RayWenderlich Style Guide
Architecture / Design Pattern
Architecture Implementation
SOLID
Single Responsibility Principle
Open Closed Principle
Liskov substitutability principle
Interface segregation
Dependency Inversion principle
Testing
Force unwrapping !
Mutable vs immutable property (Right use of
letandvar)Retain cycles (Capture list)
Code Documentation