Change the visibility of the reportParser field #1216
Conversation
|
Hi @damianszczepanik, |
| @@ -53,7 +53,7 @@ public class ReportBuilder { | |||
| private static final ObjectMapper mapper = new ObjectMapper(); | |||
|
|
|||
| private ReportResult reportResult; | |||
| private final ReportParser reportParser; | |||
| protected ReportParser reportParser; | |||
There was a problem hiding this comment.
use getter/setter method instead and add unit test into
There was a problem hiding this comment.
Thank you for your reply @damianszczepanik!
Let me try
There was a problem hiding this comment.
@damianszczepanik
I have made the required changes.
Please let me know if you have more suggestions.
| @@ -71,6 +70,20 @@ public ReportBuilder(List<String> jsonFiles, Configuration configuration) { | |||
| reportParser = new ReportParser(configuration); | |||
| } | |||
|
|
|||
| public ReportBuilder(List<String> jsonFiles, Configuration configuration, ReportParser reportParser) { | |||
There was a problem hiding this comment.
having getter and setter you don't need to add another constructor
There was a problem hiding this comment.
Indeed! it makes sense.
Let me remove the constructor.
There was a problem hiding this comment.
I have updated the description too.
| builder.setReportParser(reportParser); | ||
|
|
||
| // then | ||
| List<String> assignedJsonReports = Whitebox.getInternalState(builder, "jsonFiles"); |
There was a problem hiding this comment.
no need to validate everything here - just assert what you are trying to validate -> leave only line 8 in this section
by this this is clear that this unit test validates only one particular method while others are validated by different tests
There was a problem hiding this comment.
sure Damian! I have updated this section
| @@ -71,6 +70,14 @@ public ReportBuilder(List<String> jsonFiles, Configuration configuration) { | |||
| reportParser = new ReportParser(configuration); | |||
| } | |||
|
|
|||
| public ReportParser getReportParser() { | |||
There was a problem hiding this comment.
right, this is what I expect o extend
| // then | ||
| ReportParser assignedReportParser = Whitebox.getInternalState(builder, "reportParser"); | ||
|
|
||
| assertThat(assignedReportParser).isSameAs(builder.getReportParser()); |
There was a problem hiding this comment.
could be just
assertThat(reportParser).isSameAs(builder.getReportParser()); and no need to use Whitebox
There was a problem hiding this comment.
Duly noted Damian! Thank you.
Description
This pull request adds getter and setter methods for reportParser field with an overloaded constructor so that developers can extend the functionality of
ReportBuilderwith their own parsers, which provides greater flexibility for customized report processing.Changes
reportParserfromprivate finaltoprivate,reportParser.Motivation
In projects that require customized parsing behavior, developers may want to extend
ReportParserwith additional functionality. Adjusting the visibility ofreportParserfacilitates these extensions while maintaining the original functionality ofReportBuilder.Testing
Verified that existing functionality remains unaffected after the changes.
Confirmed that a custom parser can now be added by sub classing
ReportParserand utilized effectively.