#56 Copies logic implemented #60
Conversation
|
Job #60 is now in scope, role is |
|
This pull request #60 is assigned to @carlosmiranda/z, here is why; the budget is 15 minutes, see §4; please, read §27 and when you decide to accept the changes, inform @llorllale/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 a monetary reward for this job |
Codecov Report
@@ Coverage Diff @@
## master #60 +/- ##
============================================
- Coverage 92.85% 91.51% -1.34%
- Complexity 52 58 +6
============================================
Files 11 11
Lines 168 224 +56
Branches 11 23 +12
============================================
+ Hits 156 205 +49
- Misses 12 15 +3
- Partials 0 4 +4
Continue to review full report at Codecov.
|
|
@carlosmiranda ping |
There was a problem hiding this comment.
@Vatavuk see comments, Thanks for the reminder, apologies for the delay.
| @SuppressWarnings("PMD.AvoidInstantiatingObjectsInLoops") | ||
| private static Iterable<Copy> copies(final long id, | ||
| final Iterable<Remote> remotes) throws IOException { | ||
| final List<Copy> copies = new ArrayList<>(0); |
There was a problem hiding this comment.
@Vatavuk let's not initialize with size zero, this will force unnecessary reallocation internally for the first element. Let's use as small sensible default, like 10.
| @@ -43,6 +44,7 @@ | |||
| * wallets; Network.push must push a wallet to a remote based in remote. | |||
| * @checkstyle JavadocMethodCheck (500 lines) | |||
| * @checkstyle MagicNumberCheck (500 lines) | |||
| * @checkstyle ClassDataAbstractionCouplingCheck (500 lines) | |||
There was a problem hiding this comment.
@Vatavuk 2 lines is fine here
| * @return Boolean Boolean | ||
| * @throws IOException If fails | ||
| * @todo #56:30min Comparing of two wallets should be done in a different | ||
| * manner. Wallets should implement Bytes interface from cactoos and |
There was a problem hiding this comment.
@Vatavuk why Bytes? any explanation here would be great.
There was a problem hiding this comment.
@carlosmiranda my idea was to use Equality class from cactoos which compares byte representations of two objects. This way we don't need to explicitly compare object content parameter by parameter. Wallet just need to print itself to a byte array and Equality class will do the comparison. Wallet will need to implement Bytes because Equality operates on that interface.
More about this idea here
There was a problem hiding this comment.
@Vatavuk sorry, but I disagree with this solution:
- I don't think we have a business need to serialize wallets into bytes
- I disagree with Yegor on this one. I think it goes against the very idea of having "living objects". I can't reconcile having "living objects" turn into "dead byte streams". And like I mentioned in my previous point, I don't yet see any connection between wallets and streams of bytes.
There was a problem hiding this comment.
@llorllale Ok, I understand. My intention was to avoid writing multiple if statements. I've updated todo description
|
@carlosmiranda fixed |
|
@llorllale updated todo description |
|
@rultor merge |
@llorllale OK, I'll try to merge now. You can check the progress of the merge here |
@Vatavuk @llorllale Oops, I failed. You can see the full log here (spent 12min) |
|
@llorllale please merge again, fixed puzzle length problem. |
|
@rultor merge |
@llorllale OK, I'll try to merge now. You can check the progress of the merge here |
@llorllale Done! FYI, the full log is here (took me 13min) |
|
@ypshenychka/z please review this job completed by @carlosmiranda/z, as in §30; the job will be fully closed and all payments will be made when the quality review is completed |
|
The job #60 is now out of scope |
|
Payment to |
|
@carlosmiranda According to our QA Rules:
One out of three found issues is cosmetic. |
|
@ypshenychka confirmed |
|
@carlosmiranda thank you |
|
@0crat quality acceptable |
|
Order was finished, quality is "acceptable": +15 point(s) just awarded to @carlosmiranda/z |
|
Quality review completed: +8 point(s) just awarded to @ypshenychka/z |
Implemented logic for building copies iterable for #56.
Left two puzzles. One for wallets comparison and one for additional tests.