Conversation
| static var xBeforeAnimation: CGFloat = 0 | ||
| static var xAfterAnimation: CGFloat = 0 | ||
| static var isButtonShow = false |
There was a problem hiding this comment.
Эти переменные лучше сделать свойствами самого OverviewListViewController. Если enum называется Constants, то очевидно в него стоит помещать только константы.
isButtonShow - не очень удачный нейминг, звучит не по-английски. Можно заменить на isButtonVisible или shouldDisplayButton.
Еще о нейминге: offsetForButton станет понятнее, если заменить на buttonOffset. Предлоги лучше стараться не использовать в названиях. Это не железобетонное правило, но в большинстве случаев можно обойтись без них.
There was a problem hiding this comment.
переменные сделать свойствами VC - ✔
There was a problem hiding this comment.
Оно конечно очевидно, и была идея создать enum Variables {} Х). Про переменные лучше сделать свойствами - я запомню.
There was a problem hiding this comment.
isButtonVisible - ✔
There was a problem hiding this comment.
buttonOffset - ✔ . Предлоги лучше стараться не использовать в названиях - взял на заметку.
rowHeight , buttonHeight - ✔
| static var isButtonShow = false | ||
| } | ||
|
|
||
| private var array = [1] |
There was a problem hiding this comment.
Слишком общее название свойства. Из него непонятно что в этом массиве хранится.
Для массивов лучше явно задавать их тип, чтобы человек, читающий код не напрягался вычислением типа. Например вот так:
private var randomNumbers: [Int] = []
There was a problem hiding this comment.
private var notesArray: Results = RealmManager.shared.fetchData() - ✔
| private let createNoteButton: UIButton = { | ||
| let button: UIButton = .init() | ||
| button.setImage(UIImage(named: "pencil"), for: .normal) | ||
| button.backgroundColor = .none | ||
| button.bounds.size = .init(width: 50, height: 50) | ||
| button.addTarget(self, action: #selector(createNoteButtonPressed), for: .touchUpInside) | ||
| return button | ||
| }() |
There was a problem hiding this comment.
Вспомогательный код можно попробовать вынести в private extension. Много строк вспомогательного кода засоряет основной код и расфокусирует внимание читающего.
There was a problem hiding this comment.
вынести в private extension - ✔
|
|
||
| override func viewDidLayoutSubviews() { | ||
| super.viewDidLayoutSubviews() | ||
| Constants.isButtonShow ? setXForButton(x: Constants.xAfterAnimation) : setXForButton(x: Constants.xBeforeAnimation) |
There was a problem hiding this comment.
Мне кажется, это не очень удачное место для изменения положения кнопки. Что если изменение положения вызовет новый layout pass? Можно сделать отдельный метод updateCreateNoteButton и вызывать его там, где менятся isButtonShow или, если бы isButtonShow был свойством контроллера, то можно было бы навесить на него обсервер:
private var isButtonShow = false {
didSet { updateCreateNoteButton() }
}
Еще такой момент с неймингом: если кнопка называется createNoteButton то и все остальные сущности, которые имеют к ней отношение, должны называться похожим образом. Например, можно так переименовать:
isButtonShow -> isCreateNoteButtonVisible
setXForButton -> setXForCreateNoteButton
startButtonAnimation -> startCreteNoteButtonAnimation
| } | ||
|
|
||
| private func startButtonAnimation () { | ||
| UIView.animate(withDuration: 0.4, delay: 0.3) { [self] in |
| private func setXForButton (x: CGFloat) { | ||
| createNoteButton.frame.origin.x = x | ||
| } |
There was a problem hiding this comment.
Как-то много танцев с бубном вокруг одной кнопки в этом контроллере. Как я поняла, она должна анимированно отображаться на экране сразу после появления контроллера на экране и неанимированно скрываться при переходе на другой экран.
Можно просто сделать пару методов displayCreateNoteButtonAnimated и hideCreateNoteButton, которые и вызывать в нужных местах.
| } | ||
|
|
||
| @objc private func createNoteButtonPressed() { | ||
| array.append(Int.random(in: 11...222)) |
There was a problem hiding this comment.
Эта строчка достойна выноса в отдельный метод, из названия которого будет понятно, что тут происходит.
There was a problem hiding this comment.
Строчку удалил - была заглушкой. - ✔
| guard let controller = self.storyboard?.instantiateViewController(withIdentifier: "EditingViewController") as? EditingViewController else { return } | ||
| self.navigationController?.pushViewController(controller, animated: true) |
There was a problem hiding this comment.
Эти две строчки тоже достойны выноса в отдельный метод.
There was a problem hiding this comment.
✔ . Я держал это в голове, но лучше сразу править - если увидел кривоту - по моему это как раз слова дяди Боба.
| var isAnyButtonPressed = false | ||
|
|
||
| //Нужен для передачи данных между контроллерами | ||
| var noteData: ((_ contentNote: NSMutableAttributedString,_ dateCreateNote: Date, IndexPath?) -> Void)? |
There was a problem hiding this comment.
Для хранения данных лучше сделать модель. IndexPath - это детали реализации соседнего контроллера, они не должны просачиваться в модель. Что это означает? IndexPath - это одна из особенностей работы UITableView. Появление этой информации в модели или соседнем контроллере прибивает гвоздями существование таблицы в OverviewListViewController - замена в нем UITableView на UIStackView (например) вызовет много боли и потенциальных багов. Кроме того, заметка вовсе не ответственна за свое положение в списке, поэтому такая информация должна храниться на уровне списка, а не заметки.
|
|
||
| override func viewDidAppear(_ animated: Bool) { | ||
| super.viewDidAppear(animated) | ||
| configure() |
There was a problem hiding this comment.
Кажется, вызов configure лучше перенести в viewDidLoad. Здесь configure будет вызываться каждый раз когда контроллер появляется на экране. После перехода в контроллер выбора изображений и возврата обратно configure вызовется еще раз, панель с кнопками создастся заново и снова добавится в иерархию UI.
There was a problem hiding this comment.
✔ Даже странно что configure оказался во viewDidAppear, скорее всего поторопился и промахнулся. Согласен configure достаточно сделать один раз за жизнь ViewController-а
| override func viewDidDisappear(_ animated: Bool) { | ||
| super.viewDidDisappear(animated) | ||
| let dateCreate = Date() | ||
| noteData?(NSMutableAttributedString.init(attributedString: textNoteTextView.attributedText), dateCreate, indexPath) |
There was a problem hiding this comment.
Интересно, зачем здесь потребовался именно NSMutableAttributedString?
| deinit { | ||
| NotificationCenter.default.removeObserver(self, name: UIResponder.keyboardWillShowNotification, object: nil) | ||
| NotificationCenter.default.removeObserver(self, name: UIResponder.keyboardWillHideNotification, object: nil) | ||
| } |
There was a problem hiding this comment.
Отлично что позаботился об отписке от уведомлений!
Можно одной строкой отписаться сразу от всех:
NotificationCenter.default.removeObserver(self)
Надо внимательно посмотреть - имеет ли смысл контроллеру получать эти уведомления, когда он, например, скрыт каким-то модальным контроллером, у которого есть поле ввода и тоже выезжает клавиатура. Если в этом нет необходимости, то можно подписываться, когда контроллер появляеся на экране и отписываться когда он исчезает с экрана.
| func getTextForNote() { | ||
| if isOldNote, indexPath != nil { | ||
| // let note = dataStoreManager.fetchResultController.object(at: indexPath!) | ||
| // textNoteTextView.attributedText = note.content | ||
| } else { | ||
| textNoteTextView.text = "" | ||
| addAttributesInTextView() | ||
| } | ||
| } |
There was a problem hiding this comment.
Несмотря на то, что код закомментирован, хочу заметить что лучше избегать косвенной передачи данных. Получается, контроллер должен куда-то сходить за данными, с которыми ему нужно работать. Он становится сильно зависим от источника данных. Этой зависимости не будет, если тот, кто инициирует вывод этого контроллера на экран, сразу передаст ему все необходимые данные.
| let fontDescriptor = UIFontDescriptor.preferredFontDescriptor(withTextStyle: .body) | ||
| let boldFontDescriptor = fontDescriptor.withSymbolicTraits(.traitBold) |
There was a problem hiding this comment.
Эти дескрипторы используются повсюду, их можно вынести в константы или в методы приватного расширения.
| let boldFont = UIFont(descriptor: boldFontDescriptor!, size: 24) | ||
| let normalFont = UIFont(descriptor: fontDescriptor, size: 20) |
There was a problem hiding this comment.
Можно вынести в приватное расширение не только дескрипторы, но и создание шрифтов. Будем в каждый из методов передавать только размер шрифта:
let boldFont = makeBoldFont(size: 24)
let normalFont = makeNormalFont(size: 24)
И пусть дескрипторы останутся скрыты внутри этих методов.
| let firstParagraph = textStorage.mutableString.paragraphRange(for: NSRange(location: 0, length: 0)) | ||
| let otherParagraphs = NSString(string: getNoteContent(text: textNoteTextView.text)) |
There was a problem hiding this comment.
Тут надо подумать как убрать несимметричность. firstParagraph имеет тип NSRange, а otherParagraphs - это String. Легко перепутать. Если несимметричность не убрать или нет смысла ее убирать, то нужно скорректировать названия переменных, чтобы было понятно.
| } | ||
| } | ||
|
|
||
| @objc func reducineTheFontSize() { |
There was a problem hiding this comment.
Опечатка: reducineTheFontSize -> reduceTheFontSize
| } | ||
| } | ||
| } | ||
|
|
There was a problem hiding this comment.
В целом, как заметно улучшить код этого контроллера. Все методы, оперирующие текстом можно вынести в отдельную сущность, которой при создании передать textNoteTextView.textStorage. Для того, чтобы подсвечивать кнопки, можно сделать ей свойство (или просто набор булевых свойств), которое будет показывать какие кнопки нужно подсветить. Код контроллера за счет такого изменения сильно разгрузится и станет намного более читабельным. Логика будет отделена от UI, что очень и очень хорошо.
Вместо изменения backgroundColor кнопок можно использовать setBackgroundImage(UIImage?, for: UIControl.State) и просто переключать состояние isHighlighted у кнопок. UIImage можно сгенерировать программно с нужными цветами.
2 participants
No description provided.