Версия для печати темы Нажмите сюда для просмотра этой темы в оригинальном формате

Форум программистов > Java: Общие вопросы > качество кода

Автор: blackbanny 18.4.2013, 07:51

Доброго времени суток! Учу Java совсем недавно, в коммерческой разработке не участвовал. Хотелось бы, чтобы вы посмотрели качество кода, понятность и т.п. Стоило ли в пакете https://github.com/ZaytsevProject/DomenInformation/tree/master/src/ru/regexinfo/domeninformation создавать интерфейс, базовый класс и производный? Корректен ли интерфейс классов? Верно ли работаю с исключениями? Приложение совсем небольшое, просто хочется выслушать замечания, чтобы не допускать ошибок на более серьезных проектах  Приложение предоставляет информацию о домене. ссылка на https://github.com/ZaytsevProject/DomenInformation

Автор: LSD 18.4.2013, 10:59

Не надо коммиты нумеровать. first commit second commit это не комментарий, с таким же успехом можно оставить сообщение пустым. В сообщении должно содержаться краткое описание того, что было сделано: - create empty project - create main interface  Комитить в репозиторий скомпилированне классы не надо. Добавлено через 39 секунд Тесты должны лежать отдельно от основного кода. Добавлено через 1 минуту и 42 секунды Комментарии на языке отличном от английского - зло. Добавлено через 5 минут В JavaDoc так же надо описывать поведение в "исключительных" ситуациях. Например void setUrl(String url); что будет в случае неправильного URL? А в случае null? Добавлено через 6 минут и 27 секунд Зачем создан DomenInformationBase, планируются альтернативные реализации? Добавлено через 7 минут и 52 секунды Выделять исключения в отдельный пакадж не стоит, пусть лежат в том же в котором используются.

Автор: blackbanny 18.4.2013, 11:29

спасибо за замечания!  Цитата(LSD @  18.4.2013,  10:59 ) Зачем создан DomenInformationBase, планируются альтернативные реализации? да, планируется написать класс для парсинга информации о домене с другого сайта... а то, что в интерфейсе у меня только методы Get/Set это нормально? Может стоит класс DomenInformationFromCyPr разбить на несколько классов, т.е. чтобы не один класс предоставлял всю информацию по домену, а отдельные классы - класс для получения имени владельца домена, класс для получения даты регистрации домена и т.д.? Корректно ли логирование и обработка исключений?

Автор: LSD 18.4.2013, 12:04

Цитата(blackbanny @  18.4.2013,  12:29 ) а то, что в интерфейсе у меня только методы Get/Set это нормально? Может стоит класс DomenInformationFromCyPr разбить на несколько классов, т.е. чтобы не один класс предоставлял всю информацию по домену, а отдельные классы - класс для получения имени владельца домена, класс для получения даты регистрации домена и т.д.? Я бы сделал класс, DomainInfo который содержит в себе всю информацию о домене (включая ошибки парсинга, если возможна ситуация что информацию удалось получить лишь частично). И интерфейс DomainInfoResolver который будет на вход получать информацию о домене и возвращать DomainInfo. Цитата(blackbanny @  18.4.2013,  12:29 ) Корректно ли логирование и обработка исключений? Логгирования честно говоря вообще не заметил.

Автор: blackbanny 18.4.2013, 19:46

Цитата(LSD @  18.4.2013,  12:04 ) включая ошибки парсинга, если возможна ситуация что информацию удалось получить лишь частично а где и в каком виде ошибки бы хранили? Цитата(LSD @  18.4.2013,  12:04 ) интерфейс DomainInfoResolver который будет на вход получать информацию о домене и возвращать DomainInfo наверное не информацию о домене, а сам домен? интерфейс или класс, не совсем понял про DomainInfoResolver? Цитата(LSD @  18.4.2013,  12:04 ) Логгирования честно говоря вообще не заметил.  логирование в главном классе проекта - https://github.com/ZaytsevProject/DomenInformation/blob/master/src/domeninformation/DomenInformation.java

Автор: Samotnik 18.4.2013, 22:40

1. DomenInformationConstants будет содержать только константы? Значит лучше сделать интерфейсом и убрать public static final  2. Названия пакетов нужно продумать лучше, не желательно делать два одинаковых названия domeninformation     2.1 Наименования пакетов желательно в одном стиле. У тебя везде в ед-ом числе, а exceptions почему-то во множественном.     2.2 Если есть Main класс, я бы пакет так и назвал 3. Не используйте System.out.println, логирование ведь подключено. К тому же в одном месте применять и то и то нет смысла.: Код catch (GetSiteInfoCheckUrlException  e) {             System.out.println("Неверный адрес сайта: " + url);  //это убрать             log.log(Level.WARNING, "Exception: ", e);                    //Вместо "Exception:" нужно писать более информативно         } catch (GetSiteInfoDomenInformationException e) {             log.log(Level.WARNING, "Exception: ", e);             System.out.println("Не удалось получить информацию о домене!");         } По коду: 1. Есть стандартные ворнинги, которые IDE подсвечивает, желательно их убрать.  2. с названиями беда: пакет checkurl класс CheckUrl  метод checkingUrl.   Я бы назвал пакет util, класс UrlResolver или UrlUtil (более общее, смотря что в классе будет). 3. Вот так if (!elements.isEmpty()) break;  тоже лучше не писать. Нужны скобки и все с новой строки. Цитата(LSD @  18.4.2013,  10:59 ) Выделять исключения в отдельный пакадж не стоит, пусть лежат в том же в котором используются.  Почему? По мне, так исключения лучше выносить в свой пакет исключений.

Автор: LSD 19.4.2013, 11:06

Цитата(blackbanny @  18.4.2013,  20:46 ) а где и в каком виде ошибки бы хранили? Я плохо представляю какого рода ошибки могу быть, но в целом я бы сделал так: - если ошибка фатальная и вообще никакой информации о домене не удалось получить - выбрасываем эксепшн - если поличили информацию что такой домен не существует, создаем DomainInfo у которого ставим статус Not found (статусы я бы сделал enum-ом) - если информацию удалось получить но лишь частично создаем DomainInfo у которого ставим статус partialy resolved - если информацию удалось получить в полном объеме - то статус ОК Цитата(blackbanny @  18.4.2013,  20:46 ) наверное не информацию о домене, а сам домен? Ну да, имелся в виду URL домена. Цитата(blackbanny @  18.4.2013,  20:46 ) логирование в главном классе проекта - DomenInformation Этого явно недостаточно. Нужно еще добавать логгирование в код который парсит информацию о домене, чтобы в случае проблем понять что там происходит. Цитата(Samotnik @  18.4.2013,  23:40 ) Почему? По мне, так исключения лучше выносить в свой пакет исключений.  А смысл отделять исключение от того кода с которым оно связано? Посмотри на JDK, там исключения лежат в тех же пакаджах что и классы которые их используют.

Автор: blackbanny 19.4.2013, 11:50

спасибо за ценные замечания!  Цитата(Samotnik @  18.4.2013,  22:40 ) 1. DomenInformationConstants будет содержать только константы? Значит лучше сделать интерфейсом и убрать public static final  планируется, что еще будут константы Цитата(Samotnik @  18.4.2013,  22:40 ) 2. Названия пакетов нужно продумать лучше, не желательно делать два одинаковых названия domeninformation     2.1 Наименования пакетов желательно в одном стиле. У тебя везде в ед-ом числе, а exceptions почему-то во множественном.     2.2 Если есть Main класс, я бы пакет так и назвал переименовал Цитата(Samotnik @  18.4.2013,  22:40 ) 3. Не используйте System.out.println, логирование ведь подключено. К тому же в одном месте применять и то и то нет смысла.: System.out.println пока что делается для себя, потом вместо этого будет присвоение значений GUI-элементам Цитата(Samotnik @  18.4.2013,  22:40 ) 1. Есть стандартные ворнинги, которые IDE подсвечивает, желательно их убрать.  вроде все убрал, пришлось добавить @Override у методов и метод setUrl() в DomenInformationFromCyPr сделать final Цитата(Samotnik @  18.4.2013,  22:40 ) 2. с названиями беда: пакет checkurl класс CheckUrl  метод checkingUrl.   Я бы назвал пакет util, класс UrlResolver или UrlUtil (более общее, смотря что в классе будет). переименовал, как вы посоветовали  Цитата(Samotnik @  18.4.2013,  22:40 ) 3. Вот так if (!elements.isEmpty()) break;  тоже лучше не писать. Нужны скобки и все с новой строки. скобки расставил Сейчас, наверное, сделаю, как посоветовал сделать LSD: Цитата(LSD @  18.4.2013,  12:04 ) Я бы сделал класс, DomainInfo который содержит в себе всю информацию о домене (включая ошибки парсинга, если возможна ситуация что информацию удалось получить лишь частично) только немного не пойму, в каком виде и где хранить ошибки парсинга в этом классе? в переменных которые буду отвечать за информацию о домене, например, переменная owner содержит владельца, если не удалось спарсить владельца, то присваиваем owner = "не удалось определить владельца"; так ?  Цитата(LSD @  18.4.2013,  12:04 ) И интерфейс DomainInfoResolver который будет на вход получать информацию о домене и возвращать DomainInfo с интерфейсом не совсем понял... и может тогда класс DomenInfo сделать внутренним классом DomenInformationBase?

Автор: LSD 19.4.2013, 13:01

Цитата(blackbanny @  19.4.2013,  12:50 ) только немного не пойму, в каком виде и где хранить ошибки парсинга в этом классе? в переменных которые буду отвечать за информацию о домене, например, переменная owner содержит владельца, если не удалось спарсить владельца, то присваиваем owner = "не удалось определить владельца"; так ?  Добавляешь переменную которая содержит ParseStatus Код public enum ParseStatus {   OK,   NOT_FOUND,   PARTIALY_RESOLVED   } Цитата(blackbanny @  19.4.2013,  12:50 ) с интерфейсом не совсем понял... и может тогда класс DomenInfo сделать внутренним классом DomenInformationBase? Я имел в виду, что должен быть отдельный класс который хранит в себе информацию о домене. И отдельный класс который эту информацию получает. И раз уж у тебя планируется несколько источников получения информации, то DomainInfoResolver я предложил сделать интерфейсом.

Автор: blackbanny 25.4.2013, 16:01

переделал проект, вроде, как советовал LSD, остались только комментарии и логирование https://github.com/ZaytsevProject/DomainInfo

Powered by Invision Power Board (http://www.invisionboard.com)
© Invision Power Services (http://www.invisionpower.com)