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

org.cactoos.io.Joined: add a Ctor of Iterable<Input> #1035

Closed
ilyakharlamov opened this issue Jan 22, 2019 · 12 comments
Closed

org.cactoos.io.Joined: add a Ctor of Iterable<Input> #1035

ilyakharlamov opened this issue Jan 22, 2019 · 12 comments

Comments

@ilyakharlamov
Copy link
Contributor

ilyakharlamov commented Jan 22, 2019

org.cactoos.io.Joined in a constructor uses new ...

private final Iterable<Input> inputs;

public Joined(final Input first, final Input... rest) {
    this.inputs = new org.cactoos.iterable.Joined<>(
        first,
        new IterableOf<>(rest)
    );
}
// there are no other constructors

This is valid but calling new in a Ctor have a sign of a code smell.
I suggest adding a constructor that only takes Iterable<Input>

@0crat
Copy link
Collaborator

0crat commented Jan 22, 2019

@llorllale/z please, pay attention to this issue

@llorllale
Copy link
Contributor

@ilyakharlamov

calling new in a Ctor have a sign of a code smell.

I disagree. What makes you think this? We do this constantly in secondary constructors. You can think of them as "adapter constructors".

I suggest adding a constructor that only takes Iterable

If that's something you need, we can surely implement it.

ilyakharlamov added a commit to ilyakharlamov/cactoos that referenced this issue Jan 22, 2019
@ilyakharlamov
Copy link
Contributor Author

@llorllale There is more than one way to look at this.
For example

class A {
  private final List<String> list;
  A() {
     this.list = new ArrayList<String>;
  }
}

Using new in a constructor, relying on a concrete implementation rather than interface violates the D in SOLID principles.
Some people find it appropriate, some are not. At some point somewhere, new has to be called. Just need to be very careful about calling new inside a class that a lot of other classes depend on.

@llorllale
Copy link
Contributor

@ilyakharlamov the implementation of the class itself can depend just on the highest abstraction, while having secondary ctors for usability ("user experience")

ilyakharlamov added a commit to ilyakharlamov/cactoos that referenced this issue Jan 24, 2019
ilyakharlamov added a commit to ilyakharlamov/cactoos that referenced this issue Jan 24, 2019
ilyakharlamov added a commit to ilyakharlamov/cactoos that referenced this issue Jan 24, 2019
ilyakharlamov added a commit to ilyakharlamov/cactoos that referenced this issue Jan 25, 2019
ilyakharlamov added a commit to ilyakharlamov/cactoos that referenced this issue Jan 25, 2019
@victornoel
Copy link
Collaborator

@llorllale can we get a new release of cactoos? The work done for this issue is needed in yegor256/cactoos-http#69

@llorllale
Copy link
Contributor

@rultor release, tag is 0.39

@rultor
Copy link
Collaborator

rultor commented Feb 2, 2019

@rultor release, tag is 0.39

@llorllale OK, I will release it now. Please check the progress here

@rultor
Copy link
Collaborator

rultor commented Feb 2, 2019

@rultor release, tag is 0.39

@llorllale Done! FYI, the full log is here (took me 14min)

@victornoel
Copy link
Collaborator

@llorllale thx

@llorllale
Copy link
Contributor

@ilyakharlamov this was done by you in PR #1038

@0crat
Copy link
Collaborator

0crat commented Mar 2, 2019

Job gh:yegor256/cactoos#1035 is not assigned, can't get performer

@0crat
Copy link
Collaborator

0crat commented Mar 2, 2019

This job is not in scope

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

5 participants