Added interfaces for time and data parsing by ZeroNetto · Pull Request #5 · ZeroNetto/liquid-log-master

ZeroNetto · 2017-11-13T13:04:29Z

@ykalemi, прошу проверить.

ykalemi · 2017-11-13T18:48:08Z

src/main/java/ru/naumen/sd40/log/parser/App.java

-                    data.computeIfAbsent(key, k -> new DataSet()).parseLine(line);
-                }
-            }
+        	GCParser gcParser = new GCParser();


У вас совсем исчез SDNG парсер, теперь вместо него почему-то GC

ykalemi · 2017-11-13T18:48:46Z

src/main/java/ru/naumen/sd40/log/parser/App.java

-                    data.computeIfAbsent(key, k -> new DataSet()).parseGcLine(line);
-                }
-            }
+        	TimeParser timeParser = new TimeParser();


Вместо GC - TimeParser (не очень удачное название, не понятно что внутри)

ykalemi · 2017-11-13T18:50:00Z

src/main/java/ru/naumen/sd40/log/parser/GCParser.java

+    	try (BufferedReader br = new BufferedReader(new InputStreamReader(logs.getInputStream())))
+        {
+            String line;
+            while ((line = br.readLine()) != null)


В задании было сказано, что нужно избавиться от дублирования кода. Но у вас в итоге этот while есть теперь в каждом парсере. На самом деле он должен остаться один - только в App

ykalemi · 2017-11-14T11:40:50Z

src/main/java/ru/naumen/sd40/log/parser/App.java

-        switch (parseMode)
-        {
+        switch (parseMode){
        case "sdng":


если это все изменения, то тут ничего не изменилось. В режиме sdng работает GC-парсер, хотя для него есть режим "gc".

Если вам не очень понятно, что надо сделать - лучше задать конкретные вопросы

Да, действительно перепутаны местами, прошу прощения.

ykalemi · 2017-11-14T12:14:01Z

src/main/java/ru/naumen/sd40/log/parser/IDataParser.java

+
+import java.text.ParseException;
+
+public interface IDataParser {


ActionDoneParser, ErrorParser, GCParser, TopParser - все они должны реализовывать этот интерфейс

ykalemi · 2017-11-14T12:15:32Z

src/main/java/ru/naumen/sd40/log/parser/ITimeParser.java

+import java.text.ParseException;
+
+public interface ITimeParser {
+	public long parseTime(String line) throws ParseException;


GCTimeParser, TimeParser, а также у TopParser должен появиться свой парсер для времени - они должны реализовывать этот интерфейс

Имеется в виду, что каждый из них(GCTimeParser, TimeParser, TopParser) будет должен реализовать интерфейс ITimeParser, или что будут существовать другие классы-парсеры?

Есть еще мысль сделать по аналогии с GCParser (где существует подкласс GCTimeParser) или наоборот, избавиться от этого подкласса...

ykalemi · 2017-11-14T12:25:01Z

src/main/java/ru/naumen/sd40/log/parser/App.java

-                    int min5 = 5 * 60 * 1000;
-                    long count = time / min5;
-                    long key = count * min5;
-                    data.computeIfAbsent(key, k -> new DataSet()).parseGcLine(line);


раньше в каждом режиме (внутри каждого блока case) был вызов соответствующего данному режиму метода
parseGcLine для GC
parseLine для sdng

TopParser содержал этот цикл внутри себя.

Нужно убрать эти методы из DataSet (они всего лишь переадресуют вызов в соответствующий парсер) и прямо здесь вызывать конкретный парсер. Как вы его получите - это уже зависит от вашего решения.

Также задача была "убрать дублирование". Имелся в виду в основном цикл while, который обходит все строки файла. Он есть здесь два раза, он есть в TopParser. В конце должен остаться только один.

ykalemi

Да, это уже правильное направление.

ykalemi · 2017-11-14T19:33:38Z

src/main/java/ru/naumen/sd40/log/parser/TopParser.java

+		Matcher matcher = Pattern.compile("\\d{8}|\\d{4}-\\d{2}-\\d{2}").matcher(logs.getOriginalFilename());
+        if (matcher.find()){
+            time = prepareDate(sdf.parse(dataDate + matcher.group(1)).getTime());
+            currentSet = existing.computeIfAbsent(time, k -> new DataSet());


и не будем создавать новый датасет внутри парсера времени. Пусть это будет в вызывающем коде, где цикл while по строкам

ykalemi · 2017-11-14T19:33:45Z

src/main/java/ru/naumen/sd40/log/parser/TopParser.java

 */
-public class TopParser implements IDataParser{
-
+public class TopParser implements IDataParser, ITimeParser{


Теперь давайте распилим всё-таки этот парсер на два - отдельно время. отдельно данные.

ykalemi · 2017-11-14T19:34:13Z

src/main/java/ru/naumen/sd40/log/parser/SDNGParser.java

+ * Created by doki on 22.10.16.
+ */
+public class SDNGParser implements IDataParser{
+	public final static class SDNGTimeParser implements ITimeParser{


давайте вынесем этот класс наружу

т.е. просто убрать SDNGTimeParser?

Или сделать совсем отдельный класс...

ykalemi · 2017-11-14T19:35:15Z

src/main/java/ru/naumen/sd40/log/parser/App.java

            TopParser topParser = new TopParser(logs, data);
            topParser.configureTimeZone(timeZone);
-            topParser.parse();
+            parse(data, timeZone, logs, topParser, topParser);


метод parse во всех трёх вариантах - одинаковый. Давайте оставим только один сразу после этих блоков, т.е. в конце метода

ykalemi

Направление уже верное, только, кажется, прямо сейчас приложение вообще ничего не парсит :)

Но исправления нужны не такие серьезные - просто внимательнее отнеситесь к замечаниям

ykalemi · 2017-11-15T18:39:14Z

src/main/java/ru/naumen/sd40/log/parser/SDNGParser.java

+ */
+public class SDNGParser implements IDataParser{
+
+	private DataSet currentSet;


currentSet всегда null?

ykalemi · 2017-11-15T18:40:37Z

src/main/java/ru/naumen/sd40/log/parser/TopParser.java

-    private Pattern timeRegex = Pattern.compile("^_+ (\\S+)");
+public class TopParser implements IDataParser{
+
+	private DataSet currentSet;


тут тоже вроде бы currentSet никогда не присваивается

ykalemi · 2017-11-15T18:50:01Z

src/main/java/ru/naumen/sd40/log/parser/App.java

+			continue;
+			}
+
+			data.computeIfAbsent(time, k -> new DataSet());


тут создается новый dataset, но никак не используется. Надо присвоить это значение в переменную, а переменную передать в метод следующей строки - parseLine. Измените сигнатуру у parseLine в DataParser, чтобы он принимал строку и текущий dataset. Тогда не будет проблем с пустым currentSet во всех парсерах

ykalemi · 2017-11-15T18:51:24Z

src/main/java/ru/naumen/sd40/log/parser/TopTimeParser.java

+//    private Pattern timeRegex = Pattern.compile("^_+ (\\S+)");
+
+    public TopTimeParser(MultipartFile logs, Map<Long, DataSet> existingDataSet, String timeZone) throws IllegalArgumentException{
+        //Supports these masks in file name: YYYYmmdd, YYY-mm-dd i.e. 20161101, 2016-11-01


logs в этом классе на самом деле больше не нужен. Кроме имени файла тут вообще больше ничего не используется от файла. Поэтому лучше только имя файла и передавать

ykalemi · 2017-11-15T18:53:30Z

src/main/java/ru/naumen/sd40/log/parser/TopTimeParser.java

+            time = prepareDate(sdf.parse(dataDate + matcher.group(1)).getTime());
+            return time;
+        }
+        return 0L;


Тут есть небольшая проблема - файл top устроен иначе, чем остальные. Там идет дата (время) в одной строке, потом несколько строк данных, которые относятся к этому времени. Далее опять строка со временем, несколько строк данных.

Т.е. нужно сохранить это время, и на все последующие строки данных возвращать его. Пока не получим новое время.

Added interfaces for time and data parsing

0b4d83b

ykalemi suggested changes Nov 13, 2017

View reviewed changes

Refactoring for parse method(put it in App)

780cdba

ykalemi suggested changes Nov 14, 2017

View reviewed changes

Change sdng - gc

b20d830

ykalemi suggested changes Nov 14, 2017

View reviewed changes

implemented interfaces in appropriate parsers

7530a5a

ykalemi suggested changes Nov 14, 2017

View reviewed changes

Sawn the classes to small parts

a417ca7

ykalemi suggested changes Nov 15, 2017

View reviewed changes


		import java.text.ParseException;

		public interface IDataParser {

Comments

Conversation

ZeroNetto commented Nov 13, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ykalemi left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ykalemi left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants