#396 Add ReportData class #401
Conversation
|
This pull request #401 is assigned to @fanifieiev/z, here is why; the budget is 15 minutes, see §4; please, read §27 and when you decide to accept the changes, inform @paulodamaso/z (the architect) right in this ticket; if you decide that this PR should not be accepted ever, also inform the architect; this blog post will help you understand what is expected from a code reviewer; there will be no monetary reward for this job |
|
@fanifieiev ping |
|
@fanifieiev ping... |
|
@fanifieiev ping...again.. |
|
@fanifieiev ping... |
| ReportData(final String name, final Map<String, Object> args, final double mean, | ||
| final double sigma) { | ||
| this.metr = name; | ||
| this.args = new HashMap<>(args); |
There was a problem hiding this comment.
@HDouss It is better to use unmodifiable map here
| * @return Params | ||
| */ | ||
| public Map<String, Object> params() { | ||
| return new HashMap<>(this.args); |
There was a problem hiding this comment.
@HDouss Just returning unmodifiable map would sufficient
There was a problem hiding this comment.
@fanifieiev Ok. but I think I should remove the test for immutability or change it to expect Exceptions.
There was a problem hiding this comment.
@HDouss I would personally not create a test for ReportData, it is just a holder object, just dummy object, I would better check how much it is involved in XslReportTest
There was a problem hiding this comment.
@fanifieiev it does not harm to have such tests, especially for immutability.
| final double sigma = random.nextDouble(); | ||
| final Map<String, Object> sample = ReportDataTest.args(); | ||
| final ReportData data = new ReportData(name, sample, mean, sigma); | ||
| new Assertion<>("Must returns name", data.metric(), new IsEqual<>(name)).affirm(); |
There was a problem hiding this comment.
@HDouss It is better to reformat here and below in a way:
new Assertion<>(
"<message>",
... ,
...
).affirm();
for better readability
|
@paulodamaso ping |
| ReportData(final String name, final Map<String, Object> args, final double mean, | ||
| final double sigma) { | ||
| this.metr = name; | ||
| this.args = Collections.unmodifiableMap(new HashMap<>(args)); |
There was a problem hiding this comment.
@HDouss Let's avoid using static methods. See if we can't replace this call by some of the cactoos collections
| final double sigma = 1; | ||
| final Map<String, Object> sample = ReportDataTest.args(); | ||
| final ReportData data = new ReportData(name, sample, mean, sigma); | ||
| new Assertion<>( |
There was a problem hiding this comment.
@HDouss Please split this method into several methos containing just one assertion each
| final Map<String, Object> params = new HashMap<>(sample); | ||
| final ReportData data = new ReportData(name, params); | ||
| params.clear(); | ||
| new Assertion<>( |
There was a problem hiding this comment.
@HDouss Here also, please split it into two methods
|
@paulodamaso Thanks. Fixed. |
|
@paulodamaso ping |
|
@paulodamaso ping. |
Resolving issue #396