cactoos icon indicating copy to clipboard operation
cactoos copied to clipboard

Some object's attributes without final modifier

Open fabriciofx opened this issue 5 years ago • 10 comments
trafficstars

As you know, Cactoos follows Elegant Objects principles and one of them is immutable objects. To achieve it, you use final modifier in object's attributes. But in Cactoos code we can some of no final attributes. So, let's add final modifier to them and the attributes need to change, let's use AtomicInteger, AtomicLong, AtomicReference and so on.

java/org/cactoos/io/HeadInputStream.java:    private long processed;
java/org/cactoos/iterable/IterableOf.java:                private Unchecked<I> current = new Unchecked<>(
java/org/cactoos/iterable/RangeOf.java:                    private T value = min;
java/org/cactoos/iterator/Cycled.java:    private AtomicReference<Iterator<T>> reference;
java/org/cactoos/iterator/Joined.java:    private Iterator<T> current;
java/org/cactoos/iterator/Repeated.java:    private int repeat;
java/org/cactoos/iterator/Sliced.java:    private int current;
java/org/cactoos/iterator/Sliced.java:    private IntPredicate end;
java/org/cactoos/iterator/Sliced.java:    private void skip() {
java/org/cactoos/scalar/InheritanceLevel.java:    private int calculateLevel() {
java/org/cactoos/scalar/NumberOf.java:    private NumberEnvelope envelope;
java/org/cactoos/scalar/Solid.java:    private volatile T cache;

fabriciofx avatar Sep 06 '20 04:09 fabriciofx

@fabriciofx I can see why we would like to do that to follow more closely the "law" of EO, but before committing to it, can we explore what would that bring to us in terms of the "spirit" of EO? Do you think this will actually have an positive impact apart from having final class members?

victornoel avatar Sep 06 '20 10:09 victornoel

@victornoel Yep. Making attributes final forces immutability which has as (good) side-effect thread-safety (I think so).

fabriciofx avatar Sep 06 '20 19:09 fabriciofx

@fabriciofx but it will falsely give the impression that the object can behave well in multi threading environment while it is untrue: if the internal state is "synchonized" because it is an atomic field for example, the behaviour of a method could change its value during its execution and two concurrent execution won't make the whole method's behaviour consistent.

victornoel avatar Sep 08 '20 12:09 victornoel

@victornoel We can use these Atomic* and add in the documentation that classes are not thread-safe and left puzzles to add tests using these classes with threads. WDYT? BTW, you're accepted my suggestion in one review that does the same thing! ;)

fabriciofx avatar Sep 13 '20 12:09 fabriciofx

@fabriciofx to me there are two different unrelated questions:

  • having thread safe classes: I don't think we should be aiming for that right now, it's a huge endeavour and it doesn't really make sense for the interfaces supported by cactoos (except for the few Synced decorators already existing)
  • having extreme EO code with no non-final fields: I like the idea and I'm in favour of it, but I wonder if it's not going to far...

That being said, let's do it (I mean using no non-final fields), and see what happens in practice :)

victornoel avatar Sep 13 '20 19:09 victornoel

@0crat in

victornoel avatar Sep 13 '20 19:09 victornoel

@fabriciofx

  1. Using AtomicReference in Cycled seems to be a poor choice, Queue would be a better fit. Same goes for Repeated and Joined.
    (Probably Cycled should be constructed from as new Joined<>(new Repeated(MAX_VALUE, iter))
  2. NumberOf just needs final and the fields type should be Number not `NumberEvelope.

andreoss avatar Oct 22 '20 01:10 andreoss

@andreoss

  1. I won't problems with locks if just use a "simple" Queue? Is it work fine?
  2. Ok! Thanks!

fabriciofx avatar Nov 01 '20 23:11 fabriciofx

@0crat status

victornoel avatar Jun 12 '21 14:06 victornoel

@0crat status (here)

@victornoel This is what I know about this job in C63314D6Z, as in §32:

  • The job #1438 is in scope for 9mon
  • The role is DEV
  • The job is not assigned to anyone
  • The budget is not set yet
  • These users are banned and won't be assigned:
    • @fabriciofx/z: This user reported the ticket
  • Job footprint (restricted area)

0crat avatar Jun 12 '21 15:06 0crat