IOS-6610 add a mechanism for automatic indexation of wallets #3322
Conversation
…add-a-mechanism-for-automatic-indexation-of-wallets-3
| class UserWalletNameIndexationTests: XCTestCase { | ||
| private var existingNameTestCases: [(String, String)] { | ||
| [ | ||
| ("Wallet 2", /* -> */ "Wallet 2"), |
There was a problem hiding this comment.
если уже есть индекс его пропускаем, на дублирование не обращаем внимания? Может обсуждали где-то? в описании задачи не нашел
There was a problem hiding this comment.
Показывал Лехе результат миграции, он одобрил
There was a problem hiding this comment.
Wallet, Wallet, Wallet2 превратятся в Wallet2, Wallet3, Wallet2 ?
There was a problem hiding this comment.
Wallet -> Wallet (потому что такого нет)
Wallet -> Wallet 3 (потому что 2 уже есть)
Wallet2 -> Wallet 2 (потому что пользователь задал имя)
В первом куске тестов есть подобное
Tangem/App/Services/UserWalletRepository/CommonUserWalletRepository.swift
Outdated
Show resolved
Hide resolved
| private var indexesByNameTemplate: [String: [Int]] = [:] | ||
| private let nameComponentsRegex = try! NSRegularExpression(pattern: "^(.+)(\\s+\\d+)$") | ||
|
|
||
| init(mode: Mode, names: [String]) { |
There was a problem hiding this comment.
Давай сделаем утилиту stateless, без mode. Просто набор методов и немного подрефачим для простоты
публичные методы
- миграция
Миграция пусть работает сразу с моделями кошельков, тогда эта логика переедет из репозитория сюда и позже будет удалена.
- получили словари с занятыми индексами распарсив кошельки
- пошли в цикле по кошелькам, ищем пустые,
- находим ближайший свободный индекс
- конструируем имя и обновляем у кошелька
- обновляем локальный словарь индексов (может сет заюзать?)
- продолжаем, пока кошельки не кончатся
- возвращаем новые кошельки, если была миграция. Если не было, возвращаем nil или пустой массив, это хорошая оптимизация
- поиск нового имени
- получили словари с занятыми индексами распарсив кошельки
- взяли ближайший свободный
- сконструировали имя и вернули
Приватные
- Парсинг кошельков
- поиск следующего индекса
- конструктор имени
There was a problem hiding this comment.
Ради простоты и тестов не хочется передавать сюда модели, поэтому сделал на строках. Передавать массив строк, получать его обратно и задавать имена массиву моделей выглядело ненадежно, поэтому сделал на скалярах
There was a problem hiding this comment.
это комментарий не к тестам же
| import Foundation | ||
|
|
||
| class UserWalletNameIndexationHelper { | ||
| private var indexesByNameTemplate: [String: [Int]] = [:] |
There was a problem hiding this comment.
Что не так с indexes?
There was a problem hiding this comment.
Чаще используется форма indices, в том числе Apple, это не крит в этой задаче
There was a problem hiding this comment.
Я бы предложил оставить как есть чтобы не переусложнять
| ] | ||
| } | ||
|
|
||
| func testUserWalletNameIndexation() { |
There was a problem hiding this comment.
Давай еще пожалуйста тесты упростим, чтобы было в стиле
- тесты индексации нового кошелька
- дано имя и пользовательский набор кошельков
- ожидаемый результат такой-то
- запустили, сравнили ассертом
- тесты миграции
- дано: пользовательский массив
- ожидаемый результат
- сравнение
Все на хардкодах, без рандомных числе и тп
There was a problem hiding this comment.
Тесты фактически на хардкодах, там рандом с сидом
There was a problem hiding this comment.
тесты чем тупее написаны, тем лучше)
There was a problem hiding this comment.
В этих тестах ничего особо умного нет. Это тоже самое что 10 захардкоженных тесткейсов. Если так сильно режет глаза могу на 10 массивов заменить
There was a problem hiding this comment.
Дело не в режет. Беглого взгляда на тесты недостаточно, чтобы понять что тестируется и какой ожидаемый результат. Не хочется тратить время на ревью еще и тестов
…add-a-mechanism-for-automatic-indexation-of-wallets-3
…add-a-mechanism-for-automatic-indexation-of-wallets-3
Прошлый МР: #3204
Утилитка работает в двух режимах — миграция существующих кошельков и предложение нового имени. Пытался совместить в одно, но не получилось. Разделять на две утилиты не хочется.
Тесты с рандомайзером на всякий случай, 10 прогонов. Оба режима