adjusting the csv reader by mohammedsaad · Pull Request #3 · matthesrieke/xml-json-reader

mohammedsaad · 2019-10-24T08:45:25Z

i have done some implementation to the csv reader

i added tika to dependancy to use it for getting the mimeType

i update the getFile to toURI

update the code for readbiltiy

to ensure consisteny we used Tika as JDK 8 has a bug using Files.probeContentType

this class used to format date

this is a library to read csv

create a class that contatins a collection of the measuremants

i update date formatter to extend xml adapter that used in marshalling and unmarshal. these overided methtods are requried in JAXB

added toString method

added the annotations required for JAXB

added annotations required by JAXB with refactoring for the names of the methods

add the dependancies required by JAXB

refactor to the CSVReader

adding dateFormatter Object

update the custom date format to be public which is used in the JsonParser.java

adding Jason Property to the class variables to be recognized by Jakson Object mapper

refactoring the naming of the properties to be constant for ensuring consistency in case of changing the attributes' names

reduce redundancy of filed names and get it from the class definition

change the JABX version to solve this warning ⬇️ WARNING: Illegal reflective access by com.sun.xml.bind.v2.runtime.reflect.opt.Injector

updated the XML parser and JSON parser to throw io exception in case the given was neither path nor inputStream

add spring-boot-starter-web dependency to the pom move the class Measurement and class Measurements deom model package to Business.domain move the parser classes to business.service

update JsonReader and XmlReader so the mimeType is being refered from MediaType.APPLICATION_JSON_VALUE

matthesrieke

hi @mohammedsaad I finally was able to do the code review. We can have a chat about it tomorrow, and also plan on the next steps/tasks

matthesrieke · 2019-11-11T14:32:34Z

+    @Bean
+    public Logger getLogger(){
+        return LoggerFactory.getLogger(DatareaderApplication.class);
+    }


Good thought on using Dependency Injection for the Logger. Still, DI would not perfectly work here - at least not with a singleton Bean.

The log messages would be all coming from the same logger instance and result in something like this:

10:34:47.168 [main] INFO org.n52.datareader.DatareaderApplication: message abcdef 10:34:47.168 [main] INFO org.n52.datareader.DatareaderApplication: another message from another class

The idea of the log is to allow the people inspecting the log to identify where and when something happend (e.g. an error occurred). With the singleton bean, everything appears to happen in the DatareadApplication.class.

When using like private static final Logger LOG = LoggerFactory.getLogger(<the-class>);, the log would look like:

10:34:47.168 [main] INFO org.n52.datareader.DatareaderApplication: message abcdef 10:34:47.168 [main] INFO org.n52.datareader.web.service.XmlController: another message from another class

There are some other solutions, e.g. with some post-processing in the Beans (https://stackoverflow.com/a/6353250). These are all a little more complex, or even more "magical" then Spring already is (e.g. via Lombok).

thanks for this guidance; I have implemented it.

matthesrieke · 2019-11-11T14:38:17Z

+        CsvParser csvParser = new CsvParser();
+        try {
+            csvParser.readCsvFile(o);
+        } catch (ParseException e) {
+            e.printStackTrace();
+        }
+        measurements = csvParser.getContent();


Your CsvParser (and the other *Parser classes as well) could also be designed stateless. At the moment they have one method to parse the contents of the file. As this method is storing the result in a class field, one object of CsvParser cannot be shared by different threads (e.g. when many requests are coming into the web application). You could just return the Measurements object from the readCsvFile() method. The method would then only have method-scoped variables which makes it thread-safe.

matthesrieke · 2019-11-11T14:42:12Z

+            Spliterator<Path> F = directoryStream.spliterator();
+            StreamSupport.stream(F, true).forEach(dataFile -> {
+                try {
+                    Tika tika = new Tika();


Good use of an external library! Its always good to re-use well-tested code, and with a framework such as Tika you are good to go!

matthesrieke · 2019-11-11T14:44:36Z

+           mimeType = MediaType.APPLICATION_JSON_VALUE;
+        else if (dataType.equals("csv-data"))
+            mimeType = "text/csv";
+        else return null;


return null is in most cases not a good idea. Here, the user would get back an empty response (I guess, did not test it). It would be better to inform the user about the "exceptional" situation (which is: the application could not handle the request). E.g.:

throw new IOException("Data type not supported: " + dataType);

matthesrieke · 2019-11-11T14:48:02Z

+    public List<Measurement> readXmlData(@PathVariable("data") String dataType) throws Exception {
+        List<Measurement> measurements = null;
+        String mimeType;
+        if (dataType.equals("xml-data"))


In general, this solution of course works. Still, imho it is a better approach to use multiple methods for each URL. This is more transparent (although a bit more lines of code are created) and flexible (you could have different Content-Type ("produces") for each method:

@RequestMapping(method = RequestMethod.GET, value = "/xml-data", produces = "application/json") public List<Measurement> readXmlData() throws Exception { ...

@RequestMapping(method = RequestMethod.GET, value = "/json-data", produces = "application/json") public List<Measurement> readJsonData() throws Exception { ...

Nevertheless, now you know about @PathVariable :-)

matthesrieke · 2019-11-11T14:48:56Z

+    }
+*
+*  */
+public class DateFormatter extends XmlAdapter<String, Date> {


Good job on adding custom parsing within JAXB!

matthesrieke · 2019-11-11T14:50:03Z

+    public void readCsvFile(Object source) throws IOException, ParseException {
+        Iterable<CSVRecord> records;
+        DateFormatter dateFormatter = new DateFormatter();
+        CSVFormat csvFormat = CSVFormat.EXCEL.withDelimiter(';').withHeader().withSkipHeaderRecord();


Good use of an external library for CSV parsing.

mohammedsaad added 27 commits October 24, 2019 10:19

add tika dependancy to pom.xml

67bfff9

i added tika to dependancy to use it for getting the mimeType

fix the reading the Path

6927bb5

i update the getFile to toURI

format for readabilty of streaming the file paths

0cc9cff

update the code for readbiltiy

utilize Tika for getting mimeType

f5867eb

to ensure consisteny we used Tika as JDK 8 has a bug using Files.probeContentType

Create DateFormatter.java

d6639db

this class used to format date

add commons-csv to pom.xml

9863743

this is a library to read csv

Create Measurements.java

9ed009d

create a class that contatins a collection of the measuremants

Create CsvParser.java

7761edb

Update CsvReader.java

0b5d2d2

Update DatareaderApplication.java

73456d6

Update DateFormatter.java

816ef11

i update date formatter to extend xml adapter that used in marshalling and unmarshal. these overided methtods are requried in JAXB

Update Measurement.java

22a103e

added toString method

Update Measurement.java

455d2f9

added the annotations required for JAXB

Update Measurements.java

0d4842d

added annotations required by JAXB with refactoring for the names of the methods

Update pom.xml

c0b5183

add the dependancies required by JAXB

Update CsvReader.java

2d7ea5e

refactor to the CSVReader

Create XmlReader.java

42c46ec

Update CsvParser.java

1700436

adding dateFormatter Object

Create XmlParser.java

669d454

Create JsonParser.java

34a580f

Create JsonReader.java

6b3d742

Update pom.xml

2fb1c1b

Update DateFormatter.java

bce0136

update the custom date format to be public which is used in the JsonParser.java

Update Measurement.java

8d5332e

adding Jason Property to the class variables to be recognized by Jakson Object mapper

Update Measurement.java

f5dbb86

refactoring the naming of the properties to be constant for ensuring consistency in case of changing the attributes' names

update in CsvParser

f71be41

reduce redundancy of filed names and get it from the class definition

Update pom.xml

3221330

change the JABX version to solve this warning ⬇️ WARNING: Illegal reflective access by com.sun.xml.bind.v2.runtime.reflect.opt.Injector

mohammedsaad requested a review from matthesrieke October 25, 2019 08:21

mohammedsaad added 2 commits October 25, 2019 10:49

Update CsvParser.java

eabc827

update the parsers for consistency

6527a40

updated the XML parser and JSON parser to throw io exception in case the given was neither path nor inputStream

mohammedsaad added 3 commits November 4, 2019 10:30

restructure the project to meet webservice

c5a11bd

add spring-boot-starter-web dependency to the pom move the class Measurement and class Measurements deom model package to Business.domain move the parser classes to business.service

Create XmlController.java

f7be047

update JsonReader and XmlReader

aef667f

update JsonReader and XmlReader so the mimeType is being refered from MediaType.APPLICATION_JSON_VALUE

matthesrieke reviewed Nov 11, 2019

View reviewed changes

finalizing testing

da27298

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

adjusting the csv reader#3

adjusting the csv reader#3
mohammedsaad wants to merge 33 commits into
masterfrom
XmlReader

mohammedsaad commented Oct 24, 2019

Uh oh!

matthesrieke left a comment

Uh oh!

matthesrieke Nov 11, 2019

Uh oh!

mohammedsaad Nov 12, 2019

Uh oh!

matthesrieke Nov 11, 2019

Uh oh!

matthesrieke Nov 11, 2019

Uh oh!

matthesrieke Nov 11, 2019

Uh oh!

matthesrieke Nov 11, 2019

Uh oh!

matthesrieke Nov 11, 2019

Uh oh!

matthesrieke Nov 11, 2019

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

Conversation

mohammedsaad commented Oct 24, 2019

Uh oh!

matthesrieke 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!

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