Code Review
17.11.2009
Интерфейс
interface\AlekseevI
1. Странное имя InTrees
2. Странное имя Brick, который реализует Construction - кирпич не является сооружением
3. Нехорошие имена building1,2 - лучше бы назвать их по материалу
4. Нет функции, которая принимает аргумент-интерфейс
5. Нет юнит-тестов
AndreyZ\src\Interface
1. Почему Cars реализует IPrice? Машина является ценой?
2. numModel – что это значит? Что значит параметр num? Дурацкие ни о чём не говорящие имена
3. seePrice у машины – это что? Машина смотрит свою цену?
4. Форматировать текст не забываем
5. Константы не хардкодим (700, 500, 900, 1, 2, 3)
6. Почему в дефолтном случае возвращаем 0? Если Вам дали номер модели, который вы не знаете, это исключение, а не 0
7. Ketse пишется «КЦ»
8. Не on the storage, а in the stock
9. Очень, очень плохо, что ваш код знает о семантике другого вашего кода. Т.е., что если цена 0, то такого айтема нет. Это называется «сильное связывание» и очень, мега-плохо. Вы поменяете этот 0 на что-то ещё и все клиенты, которые на него рассчитывают, начнут работать неправильно. Если хотите узнать, есть ли айтем, сделайте функцию ItemExists. Или ловите исключение ItemDoesNotExistException
10. Нет функции, которая принимает аргумент-интерфейс
11. Юнит-тесты немного не то тестируют. Они должны были тестировать функцию, которая принимает интерфейс (например, на то, что на нулл она выдаёт исключение)
interface\AntonKarymov\src\
1. Я бы назвал MySort как-нибудь навроде ISortAlgorithm, а Bubble - BubbleSort
2. Соответственно, sortArray – sort
3. А аргумент array – arrayToSort или unsortedArray
4. helpcell – helpCell. camelCasing, не забываем
5. Никогда не кидайте NullPointerException – это прерогатива системных функций. Вам надо кидать InvalidArgument (или как он там в джаве называется).
6. Непонятно, почему sort в main проверяет array на null, а в сортировках – нет. Там же тоже паблик-функции.
interface\AntonovKirill\
1. Не забываем форматировать текст
2. Собака, кошка и корова не являются голосом
3. Функции надо называть либо «глагол» либо «глаголСуществительное». Например, sort или doIt. Функция voice – непонятно что делает
4. Нет функции, которая принимает аргумент-интерфейс
5. Нет юнит-тестов
6. Не очень хорошо держать все классы в одном файле
interface\DmitriyZ
1. Тесты и боевой код надо держать в разных проектах
2. Немного нелогично, что intOperations принимает даблы. Хотя понятно, что иначе интерфейс не сделать. Так может тогда надо было делать дженерики?
3. Если уж IntOperations принимает даблы, почему не потестировать? Что он округляет правильно?
4. А где собственно main? Где функция, принимающая интерфейс? Где тесты на эту функцию?
interface\DmitryL
1. Имена: сложение не является калькулятором. Это скорее арифметическая операция
2. Имена типа x и у запрещены. Только если это координаты
3. UnsupportedOperationException – а нет ли в джаве что-нибудь типа InvalidOperationException? Ведь деление на 0 не просто «не поддержано», оно в принципе невозможно
4. Никогда не кидайте NullPointerException – это прерогатива системных функций. Вам надо кидать InvalidArgument (или как он там в джаве называется).
interface\EugeneT
1. say – я бы называл say + что say, например, sayPreved
2. medved1, medved2 – плохие имена. Я бы назвал medved и kreved
3. Нет функции, которая принимает аргумент-интерфейс
4. Нет юнит-тестов
5. Кстати, Preved не является IMedved’ом. Kreved может быть да, но Preved точно нет
interface\Pawa
1. Имена a и b запрещены
2. convertForwards – непонятно, что это за конвертация форвардов
3. dollarRate – это должно быть константой и писаться соответственно
4. А если отрицательное количество долларов дать, это нормальная ситуация?
5. Нет функции, которая принимает аргумент-интерфейс
6. Нет юнит-тестов
7. Килограмм пишется kilogram
8. А это вообще работает? Переменная poundsInKilogramm ничем не инициализирована
interface\Savenko
1. Не забываем форматировать текст
2. br – дурацкое имя, я бы назвал reader
3. почему е1? Тогда уж е
4. questionTheUser – question – это не глагол. Надо тогда уж askTheUser. Кстати, почему the? Вы заранее не знаете, что это за юзер. Тогда уж a. А лучше просто askUser
5. "y" – константа
6. А если пользователь введёт "Y"?
7. Goodby -> Goodbye
8. mr/mrs -> Mr/Mrs
9. Не очень понятно, зачем копи-паст ввода из буферед ридера. Два класса отличаются только выводимой надписью. Остальное можно вынести в общую часть
10. Нет функции, которая принимает аргумент-интерфейс
11. Нет юнит-тестов
12. conversation1, 2 – дурацкие имена
interface\Sdmitry\
1. А где собственно код?
interface\Stepan
1. Никогда не кидайте NullPointerException – это прерогатива системных функций. Вам надо кидать InvalidArgument (или как он там в джаве называется).
2. W, H, L – такие имена запрещены. К тому же неправильно по кейсингу написаны
3. Странное имя пакета hotheart, с объёмами как-то несвязанное
4. Нет юнит-тестов на функцию calcArea в main
Исключения
Exception\AntonovKirill
- Имена типа n запрещены
- Не забываем форматировать
Exception\EugeneT
- MyExeptions -> MyExceptions
- Num1, num2 – дурацкие имена
- Не очень хорошо: человек ввёл числа, ошибся, ему сказали, мол, досвидос и вышли. Надо давать исправить ошибку. Причём если ошибся во втором числе, не заставлять заново вводить первое
- Странно, что main throws IOException, а не throws NumberFormatException. И странно, что sum его не throws.
- И странно, что вообще заново кидается исключение. Вы ведь уже вывели на экран сообщение. Зачем аварийно завершать программу?
Exception\Sdmitry
- Не очень хорошо, что в нормальной ситуации кидается исключение, которое не обрабатывается. В итоге программа аварийно завершится, вместо того, чтобы просто вывести текст про Sorry.