Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Immutable.java:40-44: Original collection should be... #1236

Closed
0pdd opened this issue Nov 12, 2019 · 20 comments
Closed

Immutable.java:40-44: Original collection should be... #1236

0pdd opened this issue Nov 12, 2019 · 20 comments

Comments

@0pdd
Copy link
Collaborator

0pdd commented Nov 12, 2019

The puzzle 1224-6c7b441d from #1224 has to be resolved:

* @todo #1224:30min Original collection should be copied inside this
* immutable decorator. That should be done because otherwise true immutability
* cannot be achieved.
* see: https://github.com/yegor256/cactoos/issues/1224 and
* https://docs.oracle.com/javase/tutorial/essential/concurrency/immutable.html

The puzzle was created by iakunin on 10-Nov-19.

Estimate: 30 minutes, role: DEV.

If you have any technical questions, don't ask me, submit new tickets instead. The task will be "done" when the problem is fixed and the text of the puzzle is removed from the source code. Here is more about PDD and about me.

@0crat
Copy link
Collaborator

0crat commented Nov 12, 2019

@paulodamaso/z please, pay attention to this issue

@0crat
Copy link
Collaborator

0crat commented Nov 12, 2019

Job #1236 is now in scope, role is DEV

@0crat 0crat added the scope label Nov 12, 2019
@0crat
Copy link
Collaborator

0crat commented Nov 12, 2019

The job #1236 assigned to @victornoel/z, here is why; the budget is 30 minutes, see §4; please, read §8 and §9; if the task is not clear, read this and this; there will be no monetary reward for this job

@victornoel
Copy link
Collaborator

@paulodamaso @iakunin I'm a bit bothered by this job (which stems fro #1224 by @quantumlexa) because it highlights something contradictory in the objectives of the collection decorators in cactoos:

  • on the one hand, the decorators (as a general pattern) are there to augment existing objects in a lazy way.
  • on the other hand, the collection decorators are actually not decorator, but new objects with their own state constructed from existing ones.

Both ways are correct, but we shouldn't mix them up.

Currently, Iterable and Iterator decorators act as real decorators (as defined above) because those two interfaces have a nice object oriented behaviour made for lazyness.

But collections, lists, sets, maps, etc, are not like that, they are meant to store data. Not to be "decorated". If you want to decorate a collection, you use the Iterable decorator and it's good enough.

So back to Immutable: what does it mean to have an Immutable decorator?

  • do we want to decorate an existing collection to be sure it's not mutable when passed to someone else ?
  • or do we want to create a new collection, that is immutable by definition?
    • and btw what sense does it make to create a new collection that implements a mutable interface just to make it immutable?
    • and also: this one can be realised by cloning the collection and decoratingh it with the Immutable (real) decorator as discussed in the point above.

The example in the original issue (#1224) that brought us here is "stupid" when looked at through the lenses of real decorators that decorate existing proper objects: of course the wrapped collection is still mutable, and it's normal, it's a mutable collection! If I open a file in read-only from Java, it won't prevent someone from modifying the file on dik, it's the same situation.

So: what should I do here? I don't agree with cloning the collection, I think Immutable should stay a decorator, and be renamed Unmodifiable to be unambiguous and consistent with how some of the other libs working on collection do. For example Eclipse Collection (which btw is a mature object oriented collection library) have an interface ImmutableCollection that has NO methods for mutability AND have implementation of Collection that are unmodifiable and behave like our Immutable.

I also think we should enforce the way the classes in the collection, list, set and map packages work so that they provide various collection implementations that play well with the decorator available in iterable but do not act as decorator because it does not make sense:

  • actually some of the classes there already follow this contract by cloning the input collection (but Immutable shouldn't be one of them)
  • but they do follow incorrectly, for example list.Sorted should maintain order upon later insertion (which is not the case currently).

I'm proposing not a full change of direction, but a clarification of the current design.

@paulodamaso if we can first decide what to do with this issue, I will then create a new issue to discuss and if accepted, organise the work around my proposal.

@victornoel
Copy link
Collaborator

@0crat wait for ARC

@0crat 0crat added the waiting label Nov 13, 2019
@0crat
Copy link
Collaborator

0crat commented Nov 13, 2019

@0crat wait for ARC (here)

@victornoel The impediment for #1236 was registered successfully by @victornoel/z

@fabriciofx
Copy link
Contributor

@victornoel @paulodamaso I think the main problem is to define what is immutability. To @yegor256 it's just to not change attributes values (they're attributed only in the constructor/construction phase, and to achieve it, we declare attributes using final keyword). To functional paradigma people, immutability is a "complete new" object. Why "complete new"? Because not all objects are copied. They use a Persistent Data Structure under the table, to achieve a perfomatic solution.
So, I think we should define what kind of immutability we're talking first. WDYT?

@victornoel
Copy link
Collaborator

@fabriciofx I'm pretty sure we are safe to say we are not talking about functional immutability until now, hence my proposal to rename the class to Unmodifiable. Then if we want to go into the field of PDS, let's do, but we should not imagine we can tackle such a domain with the interfaces from a Java collections :)

@fabriciofx
Copy link
Contributor

@victornoel to me, no problem to rename it Unmodifiable. But there is a fundamental question: one of Cactoos principles is: No mutable objects. So, collections, lists, interable should be unmodifiable by default or not? If yes, CollectionsOf, ListOf and IterableOf should return unmodifiable/immutable collections by default; if not Unmodifiable decoration makes sense. Thus, what's the way? @paulodamaso ?

@victornoel
Copy link
Collaborator

@fabriciofx

But there is a fundamental question: one of Cactoos principles is: No mutable objects. So, collections, lists, interable should be unmodifiable by default or not? If yes, CollectionsOf, ListOf and IterableOf should return unmodifiable/immutable collections by default; if not Unmodifiable decoration makes sense

See @yegor256 opinion on that matter here: #898 (comment)

Keep in mind that 100% immutability is not only impossible, but is against the spirit of OOP

More generally, I think there is a confusion here between object mutability and mutability in the domain: when you look at Java's File class, it is an immutable object, but it handles mutable concepts in its domain. This is the same for collections here IMHO.

@victornoel
Copy link
Collaborator

@paulodamaso ping?

@paulodamaso
Copy link
Contributor

paulodamaso commented Nov 25, 2019

@victornoel I think that we shgould go this way:

do we want to decorate an existing collection to be sure it's not mutable when passed to someone else ?

I just have some concerns about renaming it because we already have a lot of reference around the term Immutable, and I don't want to break this using another term.

@victornoel
Copy link
Collaborator

@paulodamaso ok, thank you, then I will remove the todo and tidy things up in terms of documentation.

@victornoel
Copy link
Collaborator

@0crat wait for #1253 to be merged

@0crat
Copy link
Collaborator

0crat commented Dec 1, 2019

@0crat wait for #1253 to be merged (here)

@victornoel Job #1236 is already on hold

victornoel added a commit to victornoel/cactoos that referenced this issue Dec 8, 2019
victornoel added a commit to victornoel/cactoos that referenced this issue Dec 8, 2019
victornoel added a commit to victornoel/cactoos that referenced this issue Dec 21, 2019
victornoel added a commit to victornoel/cactoos that referenced this issue Feb 1, 2020
victornoel added a commit to victornoel/cactoos that referenced this issue Feb 1, 2020
@0pdd 0pdd closed this as completed Feb 3, 2020
@0pdd
Copy link
Collaborator Author

0pdd commented Feb 3, 2020

The puzzle 1224-6c7b441d has disappeared from the source code, that's why I closed this issue.

@0crat 0crat removed the waiting label Feb 3, 2020
@0crat
Copy link
Collaborator

0crat commented Feb 3, 2020

@sereshqua/z please review this job completed by @victornoel/z, as in §30; the job will be fully closed and all payments will be made when the quality review is completed

@sereshqua
Copy link

@0crat quality good

@0crat 0crat removed the scope label Feb 3, 2020
@0crat
Copy link
Collaborator

0crat commented Feb 3, 2020

Order was finished, quality is "good": +35 point(s) just awarded to @victornoel/z

@0crat
Copy link
Collaborator

0crat commented Feb 3, 2020

Quality review completed: +4 point(s) just awarded to @sereshqua/z

fanifieiev pushed a commit to fanifieiev/cactoos that referenced this issue Feb 5, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

6 participants