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

TextEnvelope should be reimplemented to be a simple envelope #1287

Closed
victornoel opened this issue Feb 6, 2020 · 14 comments
Closed

TextEnvelope should be reimplemented to be a simple envelope #1287

victornoel opened this issue Feb 6, 2020 · 14 comments

Comments

@victornoel
Copy link
Collaborator

victornoel commented Feb 6, 2020

The current TextEnvelope implementation, like many of the other envelopes in cactoos, is plain wrong (see for example #1185 and other like that).

If TextEnvelope takes a Scalar, it is NOT an envelope, it's an implementation of Text based on a Scalar, and making it abstract is a mistake because 1) it promotes (bad) inheritance and 2) it can't be used directly.

I propose to:

  • Move to TextOf the current implementation of TextEnvelope.
  • make TextEnvelope just delegate asString(), toString(), equals() and hashCode() to a wrapped Text
  • redefine all other Text that previously relied on TextEnvelope behaviour via inheritance by relying on TextOf via wrapping by passing it to TextEnvelope as a superclass.

See also #942 #1156 for related tickets

@0crat
Copy link
Collaborator

0crat commented Feb 6, 2020

@paulodamaso/z please, pay attention to this issue

@victornoel
Copy link
Collaborator Author

@paulodamaso I think this should also be tackled soon, like with #1335. I just got bitten by it because TextOf nor TextEnvelope can be used when I need a Text from a Scalar while I should be able to do so.

@paulodamaso
Copy link
Contributor

@victornoel I see this "envleope" classes as default classes which have default methods for all instances; in this case, for example, it has the basic behavior for all Text instances (or should have). In theory their relation to the classes that extends them is not inheritance, but implementation / realization (according to UML theory); unfortunately java engineers decided that abstract classes have inheritance instead of realization relation to its subclasses, so in practice it have all the problems that we see in class inheritance. We could create default methods in Text interface with these behaviors, but I still don't know if it's a better solution. Just for the record, I don't like all the guys extending TextEnvelope either.

The problem I see is "envelope" seems a bad name to me. An envelope is something that wrap another thing, like an envelope and a letter; in this case we are using the name in the opposite of the real meaning: envelope is a basic / default / common implementation of Text, with methods that should be the same to every Text instance.

I'm still a bit unsure how we could solve these problems; seems that changing the code as you propose don't avoid the problem of getting wrong results on Text implementations that don't have the ScalarText wrapped inside them, for example.

@wladyan
Copy link

wladyan commented Apr 1, 2020

Lets move all system methods implementations (equals, hashCode, toString) to ComparableText decorator and create additional ctor in TextOf with Scalar.

@victornoel
Copy link
Collaborator Author

@paulodamaso

I see this "envleope" classes as default classes which have default methods for all instances; in this case, for example, it has the basic behavior for all Text instances (or should have). In theory their relation to the classes that extends them is not inheritance, but implementation / realization (according to UML theory); unfortunately java engineers decided that abstract classes have inheritance instead of realization relation to its subclasses, so in practice it have all the problems that we see in class inheritance. We could create default methods in Text interface with these behaviors, but I still don't know if it's a better solution. Just for the record, I don't like all the guys extending TextEnvelope either.

That's my whole point when I propose to add a TextOf class that replace TextEnvelope and use composition instead of inheritance.
But we still need a real envelope as defined in https://www.yegor256.com/2017/01/31/decorating-envelopes.html anyway. They correspond to what you said here:

The problem I see is "envelope" seems a bad name to me. An envelope is something that wrap another thing, like an envelope and a letter; in this case we are using the name in the opposite of the real meaning: envelope is a basic / default / common implementation of Text, with methods that should be the same to every Text instance.

@wladyan can you please edit your comment so that we know who you are talking to? :)

Lets move all system methods implementations (equals, hashCode, toString) to ComparableText decorator and create additional ctor in TextOf with Scalar.

I think TextOf can directly take this responsibility, no need for ComparableText, that's over-engineering IMHO.

@victornoel
Copy link
Collaborator Author

@0crat in

@victornoel
Copy link
Collaborator Author

@0crat assign me

@victornoel
Copy link
Collaborator Author

@0crat wait for #1458

@0pdd
Copy link
Collaborator

0pdd commented Sep 13, 2020

@victornoel 3 puzzles #1460, #1461, #1462 are still not solved.

@0crat 0crat removed the waiting label Sep 13, 2020
@0crat
Copy link
Collaborator

0crat commented Sep 13, 2020

Job was finished in 8 hours, bonus for fast delivery is possible (see §36)

@0pdd
Copy link
Collaborator

0pdd commented Dec 27, 2020

@victornoel 2 puzzles #1460, #1461 are still not solved; solved: #1462.

@0pdd
Copy link
Collaborator

0pdd commented Jan 10, 2021

@victornoel the puzzle #1461 is still not solved; solved: #1460, #1462.

@0pdd
Copy link
Collaborator

0pdd commented Mar 13, 2021

@victornoel the puzzle #1560 is still not solved; solved: #1460, #1461, #1462.

@0pdd
Copy link
Collaborator

0pdd commented May 8, 2021

@victornoel all 4 puzzles are solved here: #1460, #1461, #1462, #1560.

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

5 participants