Валидатор банковских карт

В процессе написания в целом проблем не было, но код не прошёл ревью у работодателя. Вот фидбэк: + Код покрыт Unit тестами + Видна работа по коммитам на гитхабе - В выходном фреймворке присутствуют лишние классы (SampleApp.kt) - "Caused by: android.os.NetworkOnMainThreadException", работа сети в основном потоке, вызывающая краш - Большое количество вложенных классов, что усложняет чтение и понимание кода - Единое именование реализаций ~ Избыточная сложность в использовании (~ - это доп. комментарий) ~ Классы разделены на упрощенные зоны ответственности Задачи, поставленные в тестовом написаны в тестах в шапке классов

rcd27

В выходном фреймворке присутствуют лишние классы (SampleApp.kt)

Согласен, такое можно перенести в тесты или readme. Кстати, где readme?

работа сети в основном потоке

Где-то вы друг друга не поняли. Проект ничего не знает про андроид. Либо предполагалось, что это будет Android Library, а не Java Library, либо забота об основном потоке лежит на вызывающем коде, и максимум, что может сделать автор библиотеки — написать @WorkerThread.

Большое количество вложенных классов

Речь о CardNumberValidator, Response и CardInfoService. Там немного хуже, чем просто вложенные классы — там интерфейсы ради интерфейсов.

Единое именование реализаций

Может, им не нравится, что все реализации называются *.Real? Мне тоже не нравится.

Избыточная сложность в использовании

Я бы не сказал, что прям сложно. Но многословно.

Классы разделены на упрощенные зоны ответственности

Да вроде нормально. Только там вместо классов часто достаточно отдельных функций.

Мои замечания

Условие корректности номера карты:

1. It contains only numbers and no leading 0
2. It is 12-19 digits long
3. It passes the Luhn check

Регулярка: Pattern.compile("^[^0](\\d{12,18})")

Ошибки:

• первым символом пройдёт любой не-ноль, не обязательно цифра
• должно быть 12..19 символов, без ведущего не-нуля это \d{11,18}, у тебя \d{12,18}
• регулярка компилируется на каждый вызов метода, хотя могла бы быть скомпилирована единожды
• регулярка не нужна:

private fun matchesToTheRule(cardNumber: String): Boolean =
        cardNumber.length in 12..19 &&
                cardNumber[0] != '0' &&
                cardNumber.all(Character::isDigit)

Мне не очень нравится, как вытряхиваются цифры для алгоритма Лу́на:

val digits = mutableListOf<Int>().apply {
    cardNumberWithoutLastDigit.asIterable().forEach { c ->
        add(Character.getNumericValue(c))
    }
}

Я бы сделал это без мутабельной коллекции, без боксинга, без итератора и за меньше кода:

val digits = ByteArray(cardNumberWithoutLastDigit.length) { idx ->
    Character.getNumericValue(cardNumberWithoutLastDigit[idx]).toByte()
}

Ещё на эту же тему:

sum.toString().last()

Наверное, это нормальный код, но мне больно смотреть на аллокации :)

Можно оставить в строке последние 4 бита (sum = sum and 0xF), тогда там гарантировано останется число в диапазоне [0, 15]. После этого можно легко выбросить лишний десяток: if (sum >= 10) sum -= 10.

Зачем ResponseBody.response торчит наружу? Инкапсуляция, сокрытие реализации и полиморфизм как бы намекают, что внутри ResponseBody также может быть InputStream, ByteArray, ByteByffer или Reader.

Ещё регулярка: ("$field"):("[\w\s]+")

Парсить джейсоны регулярками — это полный отстой. Корректно работает только для некоторого подмножества JSON, в данном случае — если в field нет экранирующих символов, строковое значение непустое и состоит строго из букв, цифр, подчёркиваний и пробелов, а между токенами пробелов нет.

Если код пишется исключительно для Android, можно взять JsonReader, если нет — предоставить выбор пользователю библиотеки, приняв готовые scheme и brand. (HTTP-клиент, кстати, тоже хотелось бы использовать нормальный.)

data class CreditCard(
    var scheme: String?,
    var brand: String?
)

• Только Credit? А с моей дебетовой работать не будет? Я не живу в долг :)
• Есть ли хоть какой-нибудь смысл в карте, у которой scheme или brand — null? Если нет, то они не должны быть нуллабельными.
• Может ли MasterCard вдруг превратиться в Visa? Не думаю. Тогда val, а не var.

Стиль, в котором написан метод makeRequest, довольно странный. println существует только для «Hello world» и метода main в CLI-приложениях. in — плохой идентификатор в Котлине. Место вызова тоже странное; HTTP-кодов гораздо больше, чем просто 200 и 400.

Очень стрёмный файл с одной константой. Стоит, что ли, свалить тесты из двух файлов в один.

А вообще, гораздо интереснее тестить неправильные номера карт :)