From 9eaf42aa4e37b95ae4de210c18ec6178ca71842b Mon Sep 17 00:00:00 2001 From: Nikita Semenov Date: Thu, 11 Aug 2022 14:13:21 +0300 Subject: [PATCH] fix: code review notes --- ....swift => FilterCellStateAppearance.swift} | 45 ++++-------- .../Models/FilterCellViewModelProtocol.swift | 4 +- .../DefaultFilterCellViewModel.swift | 4 +- .../Views/DefaultFilterCollectionCell.swift | 69 +++++++++++-------- .../Protocols/FilterViewModelProtocol.swift | 69 ++++++++----------- .../ViewModels/BaseFilterViewModel.swift | 29 ++++---- .../ViewModels/DefaultFilterViewModel.swift | 6 +- .../Views/BaseFiltersCollectionView.swift | 15 ++-- TIEcommerce/TIEcommerce.podspec | 1 + .../Array/Array+SafeSubscript.swift | 27 ++++++++ .../Sources/Protocols/Updatable.swift | 2 +- 11 files changed, 142 insertions(+), 129 deletions(-) rename TIEcommerce/Sources/Filters/FiltersCollectionCell/Models/{BaseFilterCellAppearance.swift => FilterCellStateAppearance.swift} (51%) create mode 100644 TISwiftUtils/Sources/Extensions/Array/Array+SafeSubscript.swift rename TIUIKitCore/Sources/UpdatableView/UpdatableView.swift => TISwiftUtils/Sources/Protocols/Updatable.swift (96%) diff --git a/TIEcommerce/Sources/Filters/FiltersCollectionCell/Models/BaseFilterCellAppearance.swift b/TIEcommerce/Sources/Filters/FiltersCollectionCell/Models/FilterCellStateAppearance.swift similarity index 51% rename from TIEcommerce/Sources/Filters/FiltersCollectionCell/Models/BaseFilterCellAppearance.swift rename to TIEcommerce/Sources/Filters/FiltersCollectionCell/Models/FilterCellStateAppearance.swift index feac01d0..785fbced 100644 --- a/TIEcommerce/Sources/Filters/FiltersCollectionCell/Models/BaseFilterCellAppearance.swift +++ b/TIEcommerce/Sources/Filters/FiltersCollectionCell/Models/FilterCellStateAppearance.swift @@ -22,42 +22,27 @@ import UIKit -open class BaseFilterCellAppearance { +public struct FilterCellStateAppearance { - public var selectedBorderColor: UIColor - public var normalBorderColor: UIColor + public let borderColor: UIColor + public let backgroundColor: UIColor + public let fontColor: UIColor - public var selectedBgColor: UIColor - public var normalBgColor: UIColor + public let borderWidth: CGFloat + public let contentInsets: UIEdgeInsets + public let cornerRadius: CGFloat - public var selectedFontColor: UIColor - public var normalFontColor: UIColor - - public var selectedBorderWidth: CGFloat - public var normalBorderWidth: CGFloat - - public var contentInsets: UIEdgeInsets - public var cornerRadius: CGFloat - - public init(selectedBorderColor: UIColor = .systemGreen, - normalBorderColor: UIColor = .lightGray, - selectedBgColor: UIColor = .white, - normalBgColor: UIColor = .lightGray, - selectedFontColor: UIColor = .systemGreen, - normalFontColor: UIColor = .black, - selectedBorderWidth: CGFloat = 1, - normalBorderWidth: CGFloat = 0, + public init(borderColor: UIColor, + backgroundColor: UIColor, + fontColor: UIColor, + borderWidth: CGFloat, contentInsets: UIEdgeInsets = .init(top: 4, left: 8, bottom: 4, right: 8), cornerRadius: CGFloat = 6) { - self.selectedBorderColor = selectedBorderColor - self.normalBorderColor = normalBorderColor - self.selectedBgColor = selectedBgColor - self.normalBgColor = normalBgColor - self.selectedFontColor = selectedFontColor - self.normalFontColor = normalFontColor - self.selectedBorderWidth = selectedBorderWidth - self.normalBorderWidth = normalBorderWidth + self.borderColor = borderColor + self.backgroundColor = backgroundColor + self.fontColor = fontColor + self.borderWidth = borderWidth self.contentInsets = contentInsets self.cornerRadius = cornerRadius } diff --git a/TIEcommerce/Sources/Filters/FiltersCollectionCell/Models/FilterCellViewModelProtocol.swift b/TIEcommerce/Sources/Filters/FiltersCollectionCell/Models/FilterCellViewModelProtocol.swift index f7e9c13b..282416d2 100644 --- a/TIEcommerce/Sources/Filters/FiltersCollectionCell/Models/FilterCellViewModelProtocol.swift +++ b/TIEcommerce/Sources/Filters/FiltersCollectionCell/Models/FilterCellViewModelProtocol.swift @@ -21,7 +21,7 @@ // public protocol FilterCellViewModelProtocol { - var id: String { get set } - var title: String { get set } + var id: String { get } + var title: String { get } var isSelected: Bool { get set } } diff --git a/TIEcommerce/Sources/Filters/FiltersCollectionCell/ViewModels/DefaultFilterCellViewModel.swift b/TIEcommerce/Sources/Filters/FiltersCollectionCell/ViewModels/DefaultFilterCellViewModel.swift index fd7d0afe..e01cae45 100644 --- a/TIEcommerce/Sources/Filters/FiltersCollectionCell/ViewModels/DefaultFilterCellViewModel.swift +++ b/TIEcommerce/Sources/Filters/FiltersCollectionCell/ViewModels/DefaultFilterCellViewModel.swift @@ -22,8 +22,8 @@ public struct DefaultFilterCellViewModel: FilterCellViewModelProtocol, Hashable { - public var id: String - public var title: String + public let id: String + public let title: String public var isSelected: Bool public init(id: String, diff --git a/TIEcommerce/Sources/Filters/FiltersCollectionCell/Views/DefaultFilterCollectionCell.swift b/TIEcommerce/Sources/Filters/FiltersCollectionCell/Views/DefaultFilterCollectionCell.swift index 82b3b606..5a6001a7 100644 --- a/TIEcommerce/Sources/Filters/FiltersCollectionCell/Views/DefaultFilterCollectionCell.swift +++ b/TIEcommerce/Sources/Filters/FiltersCollectionCell/Views/DefaultFilterCollectionCell.swift @@ -24,35 +24,41 @@ import TIUIKitCore import TIUIElements import UIKit -open class DefaultFilterCollectionCell: ContainerCollectionViewCell, - ConfigurableView { +/* - public enum SelectionState { - case normal - case selected + У тебя в кейсах по сути одно и то-же написано. Я таки рекомендую у cellAppearance хранить 2 экземпляра структуры с этими свойствами в normalAppearance и в selectedAppearance, а здесь сделать один метод в который ты будешь эту структуру передавать и применять оттуда цвета и ширину линии. + + + + + Предлагаю визуально разделить переменные, либо завести структурку FilterCellState и дальше либо Appearance или Configuration или какой-нибудь микс из них в названии. Держать там 3 свойства color, backgroundColor и fontColor. А здесь держать 2 экземпляра этой струкруты для normal и selected состояний. Ну и кажется что нет необходимости делать BaseFilterCellAppearance классом, вместо стукруры + + У тебя в инициализаторе 10 аргументов, а могло быть 4. + это упроситло бы применение апиранса. Ты просто пилишь один метод применения, и туда передаешь либо свойство normalAppearance, либо selectedAppearance и все. А внутри у тебя одинаковая реализация. + можно было бы определить набор стандартных апирансов для разных стейтов и при инициализации использовать их через .defaultSelectedAppearance, .defaultNormalAppearance и комбинировать их в нужном порядке. А сейчас нужно будет заполнять 10 аргументов инициализатора. + + Ну и остался открытым вопрос, а почему собственно BaseFilterCellAppearance класс, а не структура? И есть ли смысл делать свойства переменными, а не константами? + */ + +open class DefaultFilterCollectionCell: ContainerCollectionViewCell, ConfigurableView { + + open var selectedStateAppearance: FilterCellStateAppearance { + .defaultSelectedAppearance } - public var currentSelectionState: SelectionState { - isSelected ? .selected : .normal - } - - open var cellAppearance: BaseFilterCellAppearance { - .init() + open var normalStateAppearance: FilterCellStateAppearance { + .defaultNormalAppearance } open override var isSelected: Bool { didSet { - setAppearance() + let appearance = isSelected ? selectedStateAppearance : normalStateAppearance + updateAppearance(with: appearance) } } open override func configureAppearance() { super.configureAppearance() - layer.round(corners: .allCorners, radius: cellAppearance.cornerRadius) - contentInsets = cellAppearance.contentInsets - - setAppearance() + updateAppearance(with: normalStateAppearance) } // MARK: - ConfigurableView @@ -63,19 +69,24 @@ open class DefaultFilterCollectionCell: ContainerCollectionViewCell, // MARK: - Open methdos - open func setAppearance() { - switch currentSelectionState { - case .normal: - wrappedView.textColor = cellAppearance.normalFontColor - backgroundColor = cellAppearance.normalBgColor - layer.borderColor = cellAppearance.normalBorderColor.cgColor - layer.borderWidth = cellAppearance.normalBorderWidth + open func updateAppearance(with appearance: FilterCellStateAppearance) { + contentInsets = appearance.contentInsets + wrappedView.textColor = appearance.fontColor - case .selected: - wrappedView.textColor = cellAppearance.selectedFontColor - backgroundColor = cellAppearance.selectedBgColor - layer.borderColor = cellAppearance.selectedBorderColor.cgColor - layer.borderWidth = cellAppearance.selectedBorderWidth - } + backgroundColor = appearance.backgroundColor + layer.borderColor = appearance.borderColor.cgColor + layer.borderWidth = appearance.borderWidth + layer.round(corners: .allCorners, radius: appearance.cornerRadius) + } +} + +extension FilterCellStateAppearance { + static var defaultSelectedAppearance: FilterCellStateAppearance { + .init(borderColor: .systemGreen, backgroundColor: .white, fontColor: .systemGreen, borderWidth: 1) + } + + static var defaultNormalAppearance: FilterCellStateAppearance { + + .init(borderColor: .lightGray, backgroundColor: .lightGray, fontColor: .black, borderWidth: 0) } } diff --git a/TIEcommerce/Sources/Filters/FiltersCollectionView/Protocols/FilterViewModelProtocol.swift b/TIEcommerce/Sources/Filters/FiltersCollectionView/Protocols/FilterViewModelProtocol.swift index 6e7b98de..afc77da0 100644 --- a/TIEcommerce/Sources/Filters/FiltersCollectionView/Protocols/FilterViewModelProtocol.swift +++ b/TIEcommerce/Sources/Filters/FiltersCollectionView/Protocols/FilterViewModelProtocol.swift @@ -21,82 +21,73 @@ // import Foundation +import TISwiftUtils public protocol FilterViewModelProtocol: AnyObject { - associatedtype Property: FilterPropertyValueRepresenter & Hashable + associatedtype PropertyValue: FilterPropertyValueRepresenter & Hashable associatedtype CellViewModelType: FilterCellViewModelProtocol & Hashable - typealias Change = (indexPath: IndexPath, viewModel: CellViewModelType) + var values: [PropertyValue] { get set } + var selectedValues: [PropertyValue] { get set } - var properties: [Property] { get set } - var selectedProperties: [Property] { get set } - - func filterDidSelected(atIndexPath indexPath: IndexPath) -> [Change] - func toggleProperty(atIndexPath indexPath: IndexPath) -> (selected: [Property], deselected: [Property]) + func filterDidSelected(atIndexPath indexPath: IndexPath) -> [(indexPath: IndexPath, viewModel: CellViewModelType)] + func toggleProperty(atIndexPath indexPath: IndexPath) -> (selected: [PropertyValue], deselected: [PropertyValue]) } public extension FilterViewModelProtocol { - func toggleProperty(atIndexPath indexPath: IndexPath) -> (selected: [Property], deselected: [Property]) { - guard let item = getPropertySafely(indexPath.item) else { return ([], []) } + func toggleProperty(atIndexPath indexPath: IndexPath) -> (selected: [PropertyValue], deselected: [PropertyValue]) { + guard let item = values[safe: indexPath.item] else { return ([], []) } return toggleProperty(item) } @discardableResult - private func toggleProperty(_ property: Property) -> (selected: [Property], deselected: [Property]) { - var propertiesToDeselect = [Property]() - var propertiesToSelect = [Property]() - let selectedPropertyId = selectedProperties.firstIndex { selectedProperty in - selectedProperty.id == property.id + private func toggleProperty(_ value: PropertyValue) -> (selected: [PropertyValue], deselected: [PropertyValue]) { + var valuesToDeselect = [PropertyValue]() + var valuesToSelect = [PropertyValue]() + let selectedValueId = selectedValues.firstIndex { selectedValue in + selectedValue.id == value.id } - if let selectedPropertyId = selectedPropertyId { + if let selectedValueId = selectedValueId { // Removes previously selected filter - selectedProperties.remove(at: selectedPropertyId) - propertiesToDeselect.append(property) + selectedValues.remove(at: selectedValueId) + valuesToDeselect.append(value) } else { // Selectes unselected filter - selectedProperties.append(property) - propertiesToSelect.append(property) + selectedValues.append(value) + valuesToSelect.append(value) // If the filter has filters to exclude, these filters marks as deselected - let excludedProperties = excludeProperties(property) - propertiesToDeselect.append(contentsOf: excludedProperties) + let excludedValues = excludeValues(value) + valuesToDeselect.append(contentsOf: excludedValues) } - return (propertiesToSelect, propertiesToDeselect) + return (valuesToSelect, valuesToDeselect) } - private func excludeProperties(_ filter: Property) -> [Property] { - let propertiesIdsToExclude = filter.excludingPropertiesIds + private func excludeValues(_ values: PropertyValue) -> [PropertyValue] { + let valuesIdsToExclude = values.excludingPropertiesIds - guard !propertiesIdsToExclude.isEmpty else { + guard !valuesIdsToExclude.isEmpty else { return [] } - var excludedProperties = [Property]() + var excludedValues = [PropertyValue]() - for propertiesIdToExclude in propertiesIdsToExclude { - let propertyToExclude = selectedProperties.first { property in - property.id == propertiesIdToExclude + for valuesIdToExclude in valuesIdsToExclude { + let propertyToExclude = selectedValues.first { property in + property.id == valuesIdToExclude } if let propertyToExclude = propertyToExclude { let (_, deselected) = toggleProperty(propertyToExclude) - excludedProperties.append(contentsOf: deselected) + excludedValues.append(contentsOf: deselected) } } - return excludedProperties - } - - private func getPropertySafely(_ index: Int) -> Property? { - guard (0.. [Change] { let (selected, deselected) = toggleProperty(atIndexPath: indexPath) - let changedProperties = properties + let changedValues = values .enumerated() .filter { selected.contains($0.element) || deselected.contains($0.element) } - changedProperties.forEach { index, element in - let isSelected = selectedProperties.contains(element) + changedValues.forEach { index, element in + let isSelected = selectedValues.contains(element) setSelectedCell(atIndex: index, isSelected: isSelected) setSelectedProperty(atIndex: index, isSelected: isSelected) } - let changedItems = changedProperties + let changedItems = changedValues .map { Change(indexPath: IndexPath(item: $0.offset, section: .zero), viewModel: cellsViewModels[$0.offset]) @@ -79,10 +78,6 @@ open class BaseFilterViewModel Bool { - properties.contains(where: { $0.id == property.id }) + values[index].isSelected = isSelected } } diff --git a/TIEcommerce/Sources/Filters/FiltersCollectionView/ViewModels/DefaultFilterViewModel.swift b/TIEcommerce/Sources/Filters/FiltersCollectionView/ViewModels/DefaultFilterViewModel.swift index 77e416a5..5a668148 100644 --- a/TIEcommerce/Sources/Filters/FiltersCollectionView/ViewModels/DefaultFilterViewModel.swift +++ b/TIEcommerce/Sources/Filters/FiltersCollectionView/ViewModels/DefaultFilterViewModel.swift @@ -22,13 +22,13 @@ open class DefaultFilterViewModel: BaseFilterViewModel { - public init(filters: [DefaultFilterPropertyValue]) { - let cellsViewModel = filters.compactMap { + public init(filterPropertyValues: [DefaultFilterPropertyValue]) { + let cellsViewModel = filterPropertyValues.compactMap { DefaultFilterCellViewModel(id: $0.id, title: $0.title, isSelected: $0.isSelected) } - super.init(filters: filters, cellsViewModels: cellsViewModel) + super.init(filterPropertyValues: filterPropertyValues, cellsViewModels: cellsViewModel) } } diff --git a/TIEcommerce/Sources/Filters/FiltersCollectionView/Views/BaseFiltersCollectionView.swift b/TIEcommerce/Sources/Filters/FiltersCollectionView/Views/BaseFiltersCollectionView.swift index 5b46cafb..832ffbd1 100644 --- a/TIEcommerce/Sources/Filters/FiltersCollectionView/Views/BaseFiltersCollectionView.swift +++ b/TIEcommerce/Sources/Filters/FiltersCollectionView/Views/BaseFiltersCollectionView.swift @@ -20,15 +20,18 @@ // THE SOFTWARE. // +import TISwiftUtils import TIUIKitCore import UIKit @available(iOS 13.0, *) open class BaseFiltersCollectionView: UICollectionView, - InitializableViewProtocol, - UpdatableView, - UICollectionViewDelegate where CellType.ViewModelType: FilterCellViewModelProtocol & Hashable { + PropertyValue: FilterPropertyValueRepresenter & Hashable>: + UICollectionView, + InitializableViewProtocol, + Updatable, + UICollectionViewDelegate where CellType.ViewModelType: FilterCellViewModelProtocol & Hashable { + public enum DefaultSection: String { case main } @@ -148,9 +151,9 @@ open class BaseFiltersCollectionView.Change]) { - for change in changes { + changes.forEach { change in guard let cell = cellForItem(at: change.indexPath) as? CellType else { - continue + return } cell.configure(with: change.viewModel) diff --git a/TIEcommerce/TIEcommerce.podspec b/TIEcommerce/TIEcommerce.podspec index 481f80b3..8dc409ef 100644 --- a/TIEcommerce/TIEcommerce.podspec +++ b/TIEcommerce/TIEcommerce.podspec @@ -16,4 +16,5 @@ Pod::Spec.new do |s| s.dependency 'TINetworking', s.version.to_s s.dependency 'TIUIKitCore', s.version.to_s s.dependency 'TIUIElements', version.to_s + s.dependency 'TISwiftUtils', version.to_s end diff --git a/TISwiftUtils/Sources/Extensions/Array/Array+SafeSubscript.swift b/TISwiftUtils/Sources/Extensions/Array/Array+SafeSubscript.swift new file mode 100644 index 00000000..982f322d --- /dev/null +++ b/TISwiftUtils/Sources/Extensions/Array/Array+SafeSubscript.swift @@ -0,0 +1,27 @@ +// +// Copyright (c) 2022 Touch Instinct +// +// Permission is hereby granted, free of charge, to any person obtaining a copy +// of this software and associated documentation files (the Software), to deal +// in the Software without restriction, including without limitation the rights +// to use, copy, modify, merge, publish, distribute, sublicense, and/or sell +// copies of the Software, and to permit persons to whom the Software is +// furnished to do so, subject to the following conditions: +// +// The above copyright notice and this permission notice shall be included in +// all copies or substantial portions of the Software. +// +// THE SOFTWARE IS PROVIDED AS IS, WITHOUT WARRANTY OF ANY KIND, EXPRESS OR +// IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY, +// FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL THE +// AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER +// LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING FROM, +// OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN +// THE SOFTWARE. +// + +public extension Array { + subscript(safe index: Index) -> Element? { + self.enumerated().first(where: { $0.offset == index })?.element + } +} diff --git a/TIUIKitCore/Sources/UpdatableView/UpdatableView.swift b/TISwiftUtils/Sources/Protocols/Updatable.swift similarity index 96% rename from TIUIKitCore/Sources/UpdatableView/UpdatableView.swift rename to TISwiftUtils/Sources/Protocols/Updatable.swift index f2d9c510..f377b21c 100644 --- a/TIUIKitCore/Sources/UpdatableView/UpdatableView.swift +++ b/TISwiftUtils/Sources/Protocols/Updatable.swift @@ -20,6 +20,6 @@ // THE SOFTWARE. // -public protocol UpdatableView: AnyObject { +public protocol Updatable: AnyObject { func update() }