Покритикуйте пожалуйста учебный проект на Java

Покритикуйте пожалуйста учебный проект.

Если коротко и не касательно предметной области — есть два сервера, в каждом из которых запущено по модели (одна модель контролирует и задаёт режим работы другой). Серверы общаются между собой с помощью JSON.

Я сделал описания с картинками (в epsm-web картинок нет) к каждому пакету проекта в репозиториях, в которых описана предметная область и как работает пакет:

— epsm-core github.com/epsm/epsm-core
— epsm-web github.com/epsm/epsm-web
— epsd-core github.com/epsm/epsd-core
— epsd-web github.com/epsm/epsd-web.

У серверов есть web-интерфейсы:

— model-epsm.rhcloud.com
— dispatcher-epsm.rhcloud.com/app/history

На страницу с графиком можно попасть либо зарегистрировавшись, либо ввести на странице логина email и пароль как:
valid@email.com
123456

Из того, что для меня понятно (не касательно предметной области):

нужно сделать:
— вынести конфигурацию Logback во внешний файл.
— переделать страницы логина и регистрации (убрать таблицу). Сделанно
— отрефакторить и покрыть тестами класс UrlRequestSender. Сделанно
— переделать сериализацию / десериализацию Json. Сделанно
— убрать «магические числа». Сделанно
— логирование login/logout пользователей в Spring security.
— аутентификацию для Rest интерфейсов.
— добавить CI. Сделанно (Travis)

можно сделать, но не в первую очередь:
— проработать процесс обмена сообщениями.
— переписать страницы на java script и контроллеры чтоб не отрисовывать страницы с помощью jsp.
— сделать расчёт модели в пределах одного шага симуляции в несколько потоков.

Что по вашему мнению в проекте нужно исправить/улучшить/добавить/убрать?
На что в первую очередь стоит обратить внимание чтоб улучшить код?
Каких технологии не хватает в проекте для резюме на трейни? (по моему нужно переделать epsm-core так, чтоб там появились bean’ы с жизненным циклом из JEE)

Будьте добры, указывая на ошибки, хотя бы примерно говорить где вы встретили её в коде, а то тут >16k строк, сразу не найти. Спасибо.

LinkedIn
Допустимые теги: blockquote, a, pre, code, ul, ol, li, b, i, del.
Ctrl + Enter
Допустимые теги: blockquote, a, pre, code, ul, ol, li, b, i, del.
Ctrl + Enter

Дизайн далек от идеального. Принцип single responsibility нарушается сплошь и рядом.
Много бессмысленного кода(например AbstractPowerObjectFactory.java — зачем абстрактный класс без какой либо логики. Имя его кстати тоже не ок). Ну итд

Спасибо что заглянули. Не могли бы вы на конкретном примере подсказать где нарушается принцип single responsibility? AbstractPowerObjectFactory так и задумывался как абстрактный суперкласс для фабрик. Так как в этом пакете не используется DI в классе реализованна проверка на null в конструкторе для всех полей, которые будут во всех фабриках. Сами фабрики пока что только заглушки, потом в этот класс может ещё перейдёт какая-то общая функциональность из фабрик. Не подскажите как его лучше назвать, назначение всех его подклассов-фабрик будет создавать разные подклассы объектов PowerObject?

почему 2 класса в одном файле???почему магические числа???
После 2 классов в одном файле — закрыл

Спасибо что заглянули. Скажите, в каком файле 2 класса? Нигде такого не должно быть. Внутренние классы я тоже вроде нигде не исользовал. Если это о комментарии ниже, то это разные файлы. Подскажите подробнее про магические числа (константы?), где именно?

Точнее внутренние классы используются кое-где внутри классов с тестами для тестирования реализованных методов в абстрактных классах. Эти внутренние классы и наследуют абстрактные.

Магические числа — числа вместо констант. Любое число надо выносить в константу. Потому что когда количество использований этого числа в разных компонентах будет велико его нельзя будет изменить. Ну и соотвественно open closed принцип

Спасибо за разъяснение. Я понял где моя ошибка, в тестах местами есть числа сами по себе, в коде может в паре-тройке мест, остальные задаются с помощью класса с константами. Скажите, принцип open-closed нарушен только этими константами, или в каком-то из мест нужно использовать классы через интерфейсы (я старался всё, что может меняться использовать через интерфейсы, а то что будет расширяться — через абстрактные классы)?

Классы нало всегда разносить по одному. Потому что во- первых усложняется поиск нужного класса

Спасибо, это мне знакомо, перепроверю, может где-то случайно положил. Специально такого не делал и навскидку не могу сказать где такое может появиться (опять таки кроме внутренних классов в тестах) потому что делал по принципу один класс — одна ответственность.

Что не получилось в десериалтзации?

Я вечером подниму то, что пытался делать и детально отвечу.

С частью проблемы, а именно с сериализацией / десериализацией разобрался сам. Приятно удивлен возможностями fasterxml.jackson. Решилось тем, что я разбросал аннотации по классам, причём там есть как иерархия так и поля не примитвов. Классы для сериализации — десериализации больше не нужны. С десериализацией в Stream стиле тоже разобрался, но она соответственно теперь тоже не нужна.

Хотел-бы узнать на будующее, если всё-таки нужно будет писать кастомные десериализаторы и там будет иерархия, можно ли делать в таком стиле:

public class MessageFieldsDeserializer{
        private int year;
        private int month;
        private int dayOfMonth;
        private int hour;
        private int minute;
        private int second;
        private int nanoOfSecond;
	
	public long deserializePowerObjectId(JsonParser jParser) throws IOException{
		jParser.nextToken();
		
		return jParser.getLongValue();
	}
	
	public LocalDateTime deserializeLocalDateTime(JsonParser jParser) throws IOException{
		resetState();
		
		return parseLocalDateTime(jParser);
	}
	
	private void resetState(){
		second = 0;
		nanoOfSecond = 0;
	}
	
	private LocalDateTime parseLocalDateTime(JsonParser jParser) throws IOException{
		jParser.nextToken();
		jParser.nextToken();
		year = jParser.getIntValue();
		jParser.nextToken();
		month = jParser.getIntValue();
		jParser.nextToken();
		dayOfMonth = jParser.getIntValue();
		jParser.nextToken();
		hour = jParser.getIntValue();
		jParser.nextToken();
		minute = jParser.getIntValue();
		
		if(jParser.nextToken() != JsonToken.END_ARRAY){
			second = jParser.getIntValue();
					
			if(jParser.nextToken() != JsonToken.END_ARRAY){
				nanoOfSecond = jParser.getIntValue();
				jParser.nextToken();
			}
		}
		
		return LocalDateTime.of(year, month, dayOfMonth, hour, minute, second, nanoOfSecond);
	}
}

public class ConsumerStateJsonDeserializer extends 
		JsonDeserializer<ConsumerState>{

	private MessageFieldsDeserializer messageDeserializer = new MessageFieldsDeserializer();
	private ConsumerState state;
	private long powerObjectId;
	private float load;
	private LocalDateTime realTimeStamp;
	private LocalDateTime simulationTimeStamp;
	private Logger logger = LoggerFactory.getLogger(ConsumerStateJsonDeserializer.class);
	
	@Override
	public ConsumerState deserialize(JsonParser jParser, DeserializationContext ctx)
			throws IOException, JsonProcessingException {
		
		while(jParser.nextToken() != JsonToken.END_OBJECT){
			String name = jParser.getCurrentName();
			
			if("powerObjectId".equals(name)){
				powerObjectId = messageDeserializer.deserializePowerObjectId(jParser);
			}else if("realTimeStamp".equals(name)){
				realTimeStamp = messageDeserializer.deserializeLocalDateTime(jParser);
			}else if("simulationTimeStamp".equals(name)){
				simulationTimeStamp = messageDeserializer.deserializeLocalDateTime(jParser);
			}else{
				jParser.nextToken();
				load = jParser.getFloatValue();
			}
		}
		
		state = new ConsumerState(powerObjectId, realTimeStamp, simulationTimeStamp, load);
		
		logger.debug("Deserialized: {} from JSON.", state);
		
		return state;
	}
}

Т.е. объект, содержащий методы для десериализации общих полей и принимающих JsonParser. И в десериализаторах подклассов вызывать его методы или всё-таки для каждого подкласса нужно писать свой десериализатор полностью?

public class PowerStationStateJsonDeserializer extends JsonDeserializer<PowerStationState>{

	private PowerStationState stationState;
	private GeneratorState generatorState;
	private List<GeneratorState> generatorStatesList = new ArrayList<GeneratorState>();
	private long powerObjectId;
	private LocalDateTime realTimeStamp;
	private LocalDateTime simulationTimeStamp;
	private float frequency;
	private int generatorQuantity;
	private int generatorNumber;
	private float generationInWM;
	private Logger logger = LoggerFactory.getLogger(PowerStationStateJsonDeserializer.class);
	
	@Override
	public PowerStationState deserialize(JsonParser jParser, DeserializationContext ctxt)
			throws IOException, JsonProcessingException {
		
		resetState();
		parseJson(jParser);
		createPowerStationState();
		addGeneratorStates();
		logger.debug("Deserialized: {}." + stationState);
		
		return stationState;
	}
	
	private void resetState(){
		generatorStatesList.clear();
	}
	
	private void parseJson(JsonParser jParser) throws JsonParseException, IOException{
		while(jParser.nextToken() != JsonToken.END_OBJECT){
			String name = jParser.getCurrentName();
			
			if("powerObjectId".equals(name)){
				jParser.nextToken();
				powerObjectId = jParser.getLongValue();
			}else if("realTimeStamp".equals(name)){
				jParser.nextToken();
				realTimeStamp = LocalDateTime.parse(jParser.getText());
			}else if("simulationTimeStamp".equals(name)){
				jParser.nextToken();
				simulationTimeStamp = LocalDateTime.parse(jParser.getText());
			}else if("generatorQuantity".equals(name)){
				jParser.nextToken();
				generatorQuantity = jParser.getIntValue();
			}else if("frequency".equals(name)){
				jParser.nextToken();
				frequency = jParser.getFloatValue();
			}else if("generators".equals(name)){
				jParser.nextToken();
				while(jParser.nextToken() != JsonToken.END_OBJECT){
					parseJson(jParser);
					generatorState = new GeneratorState(generatorNumber, generationInWM);
					generatorStatesList.add(generatorState);
				}
			}else if("generationInWM".equals(name)){
				jParser.nextToken();
				generationInWM = jParser.getFloatValue();
			}else if("generatorNumber".equals(name)){
				jParser.nextToken();
				generatorNumber = jParser.getIntValue();
			}
		}
	}
	
	private void createPowerStationState(){
		stationState = new PowerStationState(powerObjectId, realTimeStamp, simulationTimeStamp,
				generatorQuantity, frequency);
	}
	
	private void addGeneratorStates(){
		for(GeneratorState state: generatorStatesList){
			stationState.addGeneratorState(state);
		}
	}
}
Но тогда получается дублирование кода десериализации общих полей во всех подклассах.

Это была одна часть проблемы. Вторая часть — у меня на локальном Tomcat это всё работает, а на RhCloud в WildFly — нет. Когда Spring контроллер получает сообщения я получаю java.lang.NoSuchMethodError где-то в десериализации. Завтра создам ещё один сервер на RhCloud, воспроизведу проблему и выложу логи ошибки. Этот останавливать не хочу — я разослал резюме и там есть на него ссылки.

Может кому будет полезно. Применительно к этому проекту не смотрел, но была такая же проблема с Tomcat, когда у него в папке с либами лежала старая версия библиотеки fasterxml, а в war файле приложения соответственно новая. При этом приложение не деплоилось пока не удалили старые файлы из папки с либами в Томкате.

Подписаться на комментарии