(#35) Distribute TkThread responsibility in multiple classes #79
Conversation
|
Job #79 is now in scope, role is |
victornoel
force-pushed
the
35-improve-tkthread
branch
from
7a55714
to
4894c3c
Compare
|
This pull request #79 is assigned to @olenagerasimova/z, here is why; the budget is 15 minutes, see §4; please, read §27 and when you decide to accept the changes, inform @g4s8/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 |
| * using junit, cactoos-matchers and fake class (no mocks). | ||
| */ | ||
| public final class ThreadIssueButtons extends MapEnvelope<String, String> { | ||
| /** |
There was a problem hiding this comment.
@victornoel should not there be empty line between class definition and ctor javadoc?
| TmResponse res = new RsText( | ||
| new ThreadIssueText(issue, thread.lastRead().toEpochMilli()) | ||
| ); | ||
| if (issue.isOpen() && issue.author().equals(user.github().users().self())) { |
There was a problem hiding this comment.
@victornoel why did you use if statement here instead of Ternary?
| * | ||
| * @since 1.0 | ||
| * @todo #35:30min Implement Unit tests for this class | ||
| * use junit, cactoos-matchers and fake class (no mocks). |
There was a problem hiding this comment.
@victornoel I think comma is missing at the end of the first todo line or the second line should start with "using"
victornoel
force-pushed
the
35-improve-tkthread
branch
from
4894c3c
to
32bb19a
Compare
|
@olenagerasimova see my new commits (I had to rebase/force push to fix the commit messages for gitlint but I added commits for your comments). I had to add a todo to continue reducing the complexity of the class because with |
|
@victornoel thanks, though now I have some doubts about |
|
@olenagerasimova I'm not sure either :) let's have @g4s8 decide if we should revert or not last commit (32bb19a). |
|
@rultor merge |
@g4s8 @victornoel Oops, I failed. You can see the full log here (spent 3min) |
victornoel
force-pushed
the
35-improve-tkthread
branch
from
32bb19a
to
2f40b83
Compare
|
@g4s8 I've rebased to fix the problem with gitlint |
|
@rultor merge |
|
@ammaratef45/z please review this job completed by @olenagerasimova/z, as in §30; the job will be fully closed and all payments will be made when the quality review is completed |
|
The job #79 is now out of scope |
|
@olenagerasimova You didn't find 3 or more problems in the PR, please confirm that you'll try to find at least 3 problems in next time. |
|
@ammaratef45 I confirm |
|
@olenagerasimova thanks |
|
@0crat quality acceptable |
|
Order was finished, quality is "acceptable": +15 point(s) just awarded to @olenagerasimova/z |
|
Quality review completed: +4 point(s) just awarded to @ammaratef45/z |
This is for #35
By distributing the various responsibility of
TkThreadin multiple class, this will ease testing and prevent us from using checkstyle exclusions.