Sprint 06 #751
Sprint 06 #751
Conversation
| @IBOutlet private var textLabel: UILabel! | ||
| @IBOutlet private var counterLabel: UILabel! | ||
| @IBOutlet private var questionTitleLabel: UILabel! | ||
| @IBOutlet weak var activityIndicator: UIActivityIndicatorView! |
There was a problem hiding this comment.
Стоит добавить private
Необходимо указывать private для того, чтобы спрятать детали реализации конкретной части функциональности,
когда они используются только внутри области объявления.
Здесь и далее в проектах, не забывай проставлять private всем свойствам и функциям, которые не планируешь использовать вне класса.
| // MARK: - Lifecycle | ||
| override func viewDidLoad() { | ||
| super.viewDidLoad() | ||
| imageView.layer.cornerRadius = 20 | ||
| questionFactory = QuestionFactory(moviesLoader: MoviesLoader(), delegate: self) |
There was a problem hiding this comment.
Не забывай следить за отступами, общепринятые отступы помогают читать код быстрее. В общем случае поможет выделить весь файл cmd+A и зажать control+i.
| // MARK: - QuestionFactoryDelegate | ||
|
|
||
| func didLoadDataFromServer() { | ||
| activityIndicator.isHidden = true |
There was a problem hiding this comment.
Лучше вынести в отдельный метод hideLoadingIndicator, по аналогии с showLoadingIndicator
There was a problem hiding this comment.
Рекомендую посмотреть на свойство hidesWhenStopped
у activityIndicator, и использовать его вместо ручного управления свойством isHidden.
Когда ты будешь вызывать startAnimating – индикатор будет появляться автоматически, соответственно на stopAnimating – скрываться, а свойство isHidden больше не понадобится, индикатор все сделает за тебя.
Важно: это свойство настраивают всего раз раз: кодом во viewDidLoad() или аналогичной галочкой у элемента на сториборде.
А строки activityIndicator.isHidden = false(true) нужно будет убрать
|
|
||
| func didLoadDataFromServer() { | ||
| activityIndicator.isHidden = true | ||
| questionFactory?.requestNextQuestion() |
There was a problem hiding this comment.
Ты можешь дополнительно показывать activityIndicator между загрузкой вопросов, потому что загрузка вопроса тоже занимает некоторое время из-за получения изображения.
Перед запросом следующего вопроса – показывать индикатор, а в момент получения – выключать анимацию.
|
|
||
| } else { | ||
| currentQuestionIndex += 1 | ||
| self.questionFactory?.requestNextQuestion() |
There was a problem hiding this comment.
Тут self необязательно использовать, поэтому лучше его убрать.
Подробнее можешь почитать в код стайле
| self.currentQuestionIndex = 0 | ||
| self.correctAnswers = 0 | ||
|
|
||
| self.questionFactory?.requestNextQuestion() |
There was a problem hiding this comment.
Если ошибка произошла из-за того что не было интернета, то данные не загрузились и пытаться запросить следующий вопрос бессмысленно. Тут правильнее снова загружать данные с сервера.
| do { | ||
| imageData = try Data(contentsOf: movie.imageURL) | ||
| } catch { | ||
| print("Failed to load image") |
There was a problem hiding this comment.
Обращу внимание, что игра состоит из вопроса о рейтинге, на который отвечает пользователь, и контекста о фильме в виде постера(картинка).
Без постера, пользователю будет затруднительно ответить на вопрос о рейтинге фильма верно.
В этом месте, функция не завершает свою работу и идет формировать вопрос без постера.
В таком случае, бизнес-логика приложения будет нарушена, потому что пользователь не сможет ответить корректно, ведь изображения нет.
Это состояние стоит обработать, например, с помощью алерта.
Не забудь, что алерт будет обновлять UI и это тоже нужно учесть (вернуть на main), ровно также, как и прерывание функции (сделать return)
| } | ||
|
|
||
|
|
||
| private let moviesLoader: MoviesLoading |
There was a problem hiding this comment.
Стоит организовывать код следующим образом:
- Публичные переменные
- IBOutlet
- Приватные переменные
- Публичные методы(сначала
initы,overrideы, и потом остальные) - Приватные методы
- IBAction
Подробнее почитать можно тут: Организация кода
| private let questionsAmount: Int = 10 | ||
| private var currentQuestion: QuizQuestion? | ||
| private var questionFactory: QuestionFactoryProtocol? | ||
| private var alertPresenter : AlertPresenter? |
There was a problem hiding this comment.
Ты нигде не создаешь alertPresenter, поэтому при вызове метода alertPresenter?.showAlert(model: alertModel) ничего не будет происходить, алерт не будет показан. Тебе необходимо инициализировать alertPresenter
| questionFactory?.requestNextQuestion() | ||
| }) | ||
|
|
||
| alertPresenter?.showAlert(model: alertModel) |
There was a problem hiding this comment.
ничего не произойдет т.к alertPresenter в этом месте nil
добавил сетку