WIP:Added a module reporting from devonfw #194
Conversation
vapadwal
changed the title
vapadwal
changed the title
vapadwal
changed the title
| import org.springframework.boot.autoconfigure.SpringBootApplication; | ||
|
|
||
| /** | ||
| * This is the entry point for unit tests. |
There was a problem hiding this comment.
If this is for Unit tests, why is it in src/main then rather than src/test?
|
|
||
| private String templatePath; | ||
|
|
||
| private HashMap<String, Object> params; |
There was a problem hiding this comment.
HashMap is a specific implementation. Use Map instead as "API".
| import java.util.List; | ||
|
|
||
| /** | ||
| * This is the interface for a simple facade to generate a report based on Jasper Reports library. |
There was a problem hiding this comment.
IMHO this is an API that abstracts from the underlying implementation. Hence Jasper Reports library is "just" the current or default implementation.
| * @param file the file where the report must be created. | ||
| * @param format - report format | ||
| */ | ||
| public void generateReport(Report report, File file, String format); |
There was a problem hiding this comment.
- omit
publickeyword inside interface - use default method delegating to generateReport with OutputStream?
- is
Filestill the state of the art type of choice or should we supportjava.nio.Path?
| * @param file the file where the report must be created. | ||
| * @param format - report format | ||
| */ | ||
| public void generateSubreport(Report masterReport, List<Report> reports, File file, String format); |
There was a problem hiding this comment.
Who. This is quite a sophisticated API. From the JavaDoc I do not really get what it will do.
I will see if I understand it when reading the implementation but JavaDoc should be more expressive.
| * @param file the file where the report must be created. | ||
| * @param format - report format | ||
| */ | ||
| public void concatenateReports(List<Report> reports, File file, String format); |
There was a problem hiding this comment.
What does concat mean in this context? If I concat two PDF files, I will get garbage.
So is the content of the reports concatenated to a single file. Is this a regular use-case. Might be cool in some cases. So I get a table of contents for the entire document similar to what we do with asciidoc and includes... Maybe the JavaDoc can again clarify the questions that obviously came to my mind here...
| @Bean | ||
| public JRAbstractExporter getExcelExporter() { | ||
|
|
||
| return new JRXlsExporter() { | ||
| @Override | ||
| public String getExporterKey() { | ||
|
|
||
| return ReportingConstants.XLS; | ||
| } | ||
| }; | ||
| } | ||
|
|
||
| /** | ||
| * Returns the {@link JRAbstractExporter} for Xls version | ||
| * | ||
| * @return | ||
| */ | ||
| @Bean | ||
| public JRAbstractExporter getPdfExporter() { | ||
|
|
||
| return new JRPdfExporter() { |
There was a problem hiding this comment.
...
This looks like we turned a plugable concept of jasper into a monolithic approach. Does not seem to be smart to me. Am I missing something...?!? And if Jasper is providing such exporters as plugins like JRXlsExporter and JRPdfExporter, etc. why on earth do we have to override getExporterKey() method from outside. This appears strange to me. However, I am reviewing Jasper now while I actually should review this devon4j module instead...
There was a problem hiding this comment.
Also interesting that for getPdfExporter we have JavaDoc while for the others we dont. Further again this is the JavaDoc antipattern of
* Returns ...
* @return
So simply write what the method returns after the @return tag withour the Returns. This is how JavaDoc works. I know that you already know and just took over the code from devon repo. However, I am seeing this a lot in reviews. Maybe we should create a better DoD or do some trainings for all the involved coders to avoid talking about basics in reviews.
vapadwal
changed the title
Currently its WIP because there is an thread going on discussion, should we include it in devon4j
see https://www.yammer.com/capgemini.com/#/threads/show?threadId=512250200367104&messageId=512250200367104