#1445: Reorganise classes around Scalar and Callable #1494
Conversation
Codecov Report
@@ Coverage Diff @@
## master #1494 +/- ##
============================================
- Coverage 89.38% 89.36% -0.03%
+ Complexity 1630 1628 -2
============================================
Files 279 277 -2
Lines 3909 3911 +2
Branches 211 211
============================================
+ Hits 3494 3495 +1
- Misses 382 384 +2
+ Partials 33 32 -1
Continue to review full report at Codecov.
|
| */ | ||
| public ScalarOf(final Scalar<T> origin) { | ||
| this.origin = origin; | ||
| public ScalarOf(final Func<X, T> fnc, final X ipt) { |
There was a problem hiding this comment.
@fabriciofx Func & Proc parameters should follow PECS (not sure about `Scalar``).
| return new Repeated<>( | ||
| new FuncOf<>(this.callable), | ||
| public T value() throws Exception { | ||
| return new org.cactoos.func.Repeated<>( |
There was a problem hiding this comment.
@fabriciofx Can this be moved to the primary ctor?
|
|
||
| /** | ||
| * ScalarOf. | ||
| * | ||
| * @param <X> Element type |
There was a problem hiding this comment.
@fabriciofx This type parameter should be on ctor level.
There was a problem hiding this comment.
@andreoss Can you explain it better? Shoul I add X to ctor, as public <X> ScalarOf(final Runnable runnable, final T result)?
There was a problem hiding this comment.
@fabriciofx Exactly, it should be like that
public <X> ScalarOf(final Func<? super X, ? extends T> fnc, final X ipt) {
There was a problem hiding this comment.
@andreoss done, and thanks for the help! :)
| */ | ||
| public ScalarOf(final Scalar<T> origin) { | ||
| this.origin = origin; | ||
| public ScalarOf(final Func<? super X, T> fnc, final X ipt) { |
There was a problem hiding this comment.
@fabriciofx Return type should be ? extends T
| * @since 0.49.2 | ||
| */ | ||
| public final class RepeatedCallable<X> implements Callable<X> { | ||
|
|
||
| public final class Repeated<T> implements Scalar<T> { |
There was a problem hiding this comment.
@fabriciofx Not realated to the review. But what is the use case for such class?
It seems that it applies some side effects, which is bad for Scalar to have
For example:
new Repeated<>(new Count<>(1), 5)).value() == 6 (all other values lost)
There was a problem hiding this comment.
@andreoss Sorry, but I didn't get the point. I did some code here to test and works fine (but I can't make Count<T> because, I can't increment something like T:
@Test
void countFiveTimes() throws Exception {
new Assertion<>(
"Must repeat count 5 times",
new Repeated<>(
new Count(1),
5
),
new ScalarHasValue<>(5)
);
}
final class Count implements Scalar<Integer> {
private AtomicInteger num;
public Count(final int num) {
this.num = new AtomicInteger(num);
}
@Override
public Integer value() throws Exception {
return this.num.incrementAndGet();
}
}
There was a problem hiding this comment.
@fabriciofx I mean, that with Repeat only the last result is returned, and all of other iteratatios are lost. Which implies mutable state.
So Repeated may be seen as an encouragement to put mutable state in a Scalar, as in your Count example.
|
@andreoss Please, take a new look. |
|
@fabriciofx I think we should keep callableof but keep it the simplest as possible (taking a scalar), can you revert that and the changes related? :) |
|
@fabriciofx ping :) |
|
@fabriciofx can you take a look at the last comments above? |
|
@fabriciofx one last call before closing this MR, if you don't plan to tackle it just tell me so that we don't loose more time :) |
|
@fabriciofx @andreoss closing this as there are many small problems and I am not getting any feedbacks |
|
@sereshqua/z please review this job completed by @andreoss/z, as in §30; the job will be fully closed and all payments will be made when the quality review is completed |
|
Code review was too long (59 days), architects (@victornoel) were penalized, see §55 |
|
@0crat quality acceptable |
Per #1445 :
ScalarOfRepeatedCallabletoRepeatedScalarOfCallableCallableOf