Minimize amount of inner classes, predicates, tuples, etc

[Vladimir Ozerov]

Igniters,

Ignite is known to be complex product. Partially this complexity comes from
... complex problems we are trying to resolve. But partially it comes from
how we write our code. I noticed several anti-patterns which makes our code
hard to manage:

1) Inner classes (both static and non-static)
2) Usage of generic tuples
3) Usage of many classes from our "lang"package - closures, predicates,
etc..

When combined these anti-patterns makes our classes huge, leaky in terms of
abstractions, hard to follow and refactor. I would like to propose to
restrict usages of these constructs as much as possible in non-test code.
Classes should be top-level by default, unless "inner" semantics are
absolutely necessary or saves a lot of code. Tuples and closures should be
replaced with concrete classes, specific to your case.

Thoughts?

Vladimir.

[daradurvs]

Vladimir, thank you for this suggestion.
As newcomer in the Ignite I agree with you, sometimes it is very hard to
understand huge classes which contain sets of inner classes.

>> 1) Inner classes (both static and non-static)
Completelly agree, especially classes which will be serialized.

>> 2) Usage of generic tuples
Why or in what cases?

>> Tuples and closures should be replaced with concrete classes, specific
to your case.
Can you give an example?

2017-05-16 13:01 GMT+03:00 Vladimir Ozerov <[hidden email]>:

> Igniters,
>
> Ignite is known to be complex product. Partially this complexity comes from
> ... complex problems we are trying to resolve. But partially it comes from
> how we write our code. I noticed several anti-patterns which makes our code
> hard to manage:
>
> 1) Inner classes (both static and non-static)
> 2) Usage of generic tuples
> 3) Usage of many classes from our "lang"package - closures, predicates,
> etc..
>
> When combined these anti-patterns makes our classes huge, leaky in terms of
> abstractions, hard to follow and refactor. I would like to propose to
> restrict usages of these constructs as much as possible in non-test code.
> Classes should be top-level by default, unless "inner" semantics are
> absolutely necessary or saves a lot of code. Tuples and closures should be
> replaced with concrete classes, specific to your case.
>
> Thoughts?
>
> Vladimir.
>

--
Best Regards, Vyacheslav

[Vladimir Ozerov]

Vyacheslav,

When you analyze code full of generic stuff like tuples and closures, it is
much harder to understand what is going on and navigate the code.

Good coding: GridDhtAtomicCache - specific interface for callback. Easy to
navigate and understand.

interface UpdateReplyClosure extends
CI2<GridNearAtomicAbstractUpdateRequest, GridNearAtomicUpdateResponse> {
    ...
}

Bad coding: CacheContinuousQueryHandler.AcknowledgeBuffer - overcomplicated
generic, no JavaDocs, nullable semantics. Complexity out of nothing.

@Nullable synchronized IgniteBiTuple<Map<Integer, Long>,
Set<AffinityTopologyVersion>> onAcknowledged(GridContinuousBatch batch) {
    ...
}


On Tue, May 16, 2017 at 1:22 PM, Vyacheslav Daradur <[hidden email]>
wrote:

> Vladimir, thank you for this suggestion.
> As newcomer in the Ignite I agree with you, sometimes it is very hard to
> understand huge classes which contain sets of inner classes.
>
> >> 1) Inner classes (both static and non-static)
> Completelly agree, especially classes which will be serialized.
>
> >> 2) Usage of generic tuples
> Why or in what cases?
>
> >> Tuples and closures should be replaced with concrete classes, specific
> to your case.
> Can you give an example?
>
> 2017-05-16 13:01 GMT+03:00 Vladimir Ozerov <[hidden email]>:
>
> > Igniters,
> >
> > Ignite is known to be complex product. Partially this complexity comes
> from
> > ... complex problems we are trying to resolve. But partially it comes
> from
> > how we write our code. I noticed several anti-patterns which makes our
> code
> > hard to manage:
> >
> > 1) Inner classes (both static and non-static)
> > 2) Usage of generic tuples
> > 3) Usage of many classes from our "lang"package - closures, predicates,
> > etc..
> >
> > When combined these anti-patterns makes our classes huge, leaky in terms
> of
> > abstractions, hard to follow and refactor. I would like to propose to
> > restrict usages of these constructs as much as possible in non-test code.
> > Classes should be top-level by default, unless "inner" semantics are
> > absolutely necessary or saves a lot of code. Tuples and closures should
> be
> > replaced with concrete classes, specific to your case.
> >
> > Thoughts?
> >
> > Vladimir.
> >
>
>
>
> --
> Best Regards, Vyacheslav
>

[daradurvs]

Vladimir,

Thank you for explanation and examples, it is clearly now.

Maybe it will be great to add your suggestions with explanation and
examples to the article[1] "Coding guidelines"

[1] https://cwiki.apache.org/confluence/display/IGNITE/Coding+Guidelines

2017-05-17 11:08 GMT+03:00 Vladimir Ozerov <[hidden email]>:

> Vyacheslav,
>
> When you analyze code full of generic stuff like tuples and closures, it is
> much harder to understand what is going on and navigate the code.
>
> Good coding: GridDhtAtomicCache - specific interface for callback. Easy to
> navigate and understand.
>
> interface UpdateReplyClosure extends
> CI2<GridNearAtomicAbstractUpdateRequest, GridNearAtomicUpdateResponse> {
>     ...
> }
>
> Bad coding: CacheContinuousQueryHandler.AcknowledgeBuffer -
> overcomplicated
> generic, no JavaDocs, nullable semantics. Complexity out of nothing.
>
> @Nullable synchronized IgniteBiTuple<Map<Integer, Long>,
> Set<AffinityTopologyVersion>> onAcknowledged(GridContinuousBatch batch) {
>     ...
> }
>
>
> On Tue, May 16, 2017 at 1:22 PM, Vyacheslav Daradur <[hidden email]>
> wrote:
>
> > Vladimir, thank you for this suggestion.
> > As newcomer in the Ignite I agree with you, sometimes it is very hard to
> > understand huge classes which contain sets of inner classes.
> >
> > >> 1) Inner classes (both static and non-static)
> > Completelly agree, especially classes which will be serialized.
> >
> > >> 2) Usage of generic tuples
> > Why or in what cases?
> >
> > >> Tuples and closures should be replaced with concrete classes, specific
> > to your case.
> > Can you give an example?
> >
> > 2017-05-16 13:01 GMT+03:00 Vladimir Ozerov <[hidden email]>:
> >
> > > Igniters,
> > >
> > > Ignite is known to be complex product. Partially this complexity comes
> > from
> > > ... complex problems we are trying to resolve. But partially it comes
> > from
> > > how we write our code. I noticed several anti-patterns which makes our
> > code
> > > hard to manage:
> > >
> > > 1) Inner classes (both static and non-static)
> > > 2) Usage of generic tuples
> > > 3) Usage of many classes from our "lang"package - closures, predicates,
> > > etc..
> > >
> > > When combined these anti-patterns makes our classes huge, leaky in
> terms
> > of
> > > abstractions, hard to follow and refactor. I would like to propose to
> > > restrict usages of these constructs as much as possible in non-test
> code.
> > > Classes should be top-level by default, unless "inner" semantics are
> > > absolutely necessary or saves a lot of code. Tuples and closures should
> > be
> > > replaced with concrete classes, specific to your case.
> > >
> > > Thoughts?
> > >
> > > Vladimir.
> > >
> >
> >
> >
> > --
> > Best Regards, Vyacheslav
> >
>

--
Best Regards, Vyacheslav

[voipp]

yeah, good proposal! In addition i would suggest start adding more javadocs
on current code

ср, 17 мая 2017 г. в 11:08, Vladimir Ozerov <[hidden email]>:

> Vyacheslav,
>
> When you analyze code full of generic stuff like tuples and closures, it is
> much harder to understand what is going on and navigate the code.
>
> Good coding: GridDhtAtomicCache - specific interface for callback. Easy to
> navigate and understand.
>
> interface UpdateReplyClosure extends
> CI2<GridNearAtomicAbstractUpdateRequest, GridNearAtomicUpdateResponse> {
>     ...
> }
>
> Bad coding: CacheContinuousQueryHandler.AcknowledgeBuffer - overcomplicated
> generic, no JavaDocs, nullable semantics. Complexity out of nothing.
>
> @Nullable synchronized IgniteBiTuple<Map<Integer, Long>,
> Set<AffinityTopologyVersion>> onAcknowledged(GridContinuousBatch batch) {
>     ...
> }
>
>
> On Tue, May 16, 2017 at 1:22 PM, Vyacheslav Daradur <[hidden email]>
> wrote:
>
> > Vladimir, thank you for this suggestion.
> > As newcomer in the Ignite I agree with you, sometimes it is very hard to
> > understand huge classes which contain sets of inner classes.
> >
> > >> 1) Inner classes (both static and non-static)
> > Completelly agree, especially classes which will be serialized.
> >
> > >> 2) Usage of generic tuples
> > Why or in what cases?
> >
> > >> Tuples and closures should be replaced with concrete classes, specific
> > to your case.
> > Can you give an example?
> >
> > 2017-05-16 13:01 GMT+03:00 Vladimir Ozerov <[hidden email]>:
> >
> > > Igniters,
> > >
> > > Ignite is known to be complex product. Partially this complexity comes
> > from
> > > ... complex problems we are trying to resolve. But partially it comes
> > from
> > > how we write our code. I noticed several anti-patterns which makes our
> > code
> > > hard to manage:
> > >
> > > 1) Inner classes (both static and non-static)
> > > 2) Usage of generic tuples
> > > 3) Usage of many classes from our "lang"package - closures, predicates,
> > > etc..
> > >
> > > When combined these anti-patterns makes our classes huge, leaky in
> terms
> > of
> > > abstractions, hard to follow and refactor. I would like to propose to
> > > restrict usages of these constructs as much as possible in non-test
> code.
> > > Classes should be top-level by default, unless "inner" semantics are
> > > absolutely necessary or saves a lot of code. Tuples and closures should
> > be
> > > replaced with concrete classes, specific to your case.
> > >
> > > Thoughts?
> > >
> > > Vladimir.
> > >
> >
> >
> >
> > --
> > Best Regards, Vyacheslav
> >
>

--

*Best Regards,*

*Kuznetsov Aleksey*

[Dmitriy Pavlov]

Hi Vladimir,

Do you have any idea how this rules may be automatically controlled by, for
example, code inspections and/or TeamCity?

I do beleive if rule is controlled by some automatic way (not only by
argreement) it has a chance to become true practice in real life.

If this excellent idea remains only agreement on dev list, it may be
forgotten in the future.

Sincerely,
Dmitriy Pavlov

вт, 16 мая 2017 г. в 13:01, Vladimir Ozerov <[hidden email]>:

> Igniters,
>
> Ignite is known to be complex product. Partially this complexity comes from
> ... complex problems we are trying to resolve. But partially it comes from
> how we write our code. I noticed several anti-patterns which makes our code
> hard to manage:
>
> 1) Inner classes (both static and non-static)
> 2) Usage of generic tuples
> 3) Usage of many classes from our "lang"package - closures, predicates,
> etc..
>
> When combined these anti-patterns makes our classes huge, leaky in terms of
> abstractions, hard to follow and refactor. I would like to propose to
> restrict usages of these constructs as much as possible in non-test code.
> Classes should be top-level by default, unless "inner" semantics are
> absolutely necessary or saves a lot of code. Tuples and closures should be
> replaced with concrete classes, specific to your case.
>
> Thoughts?
>
> Vladimir.
>