2836 cage stackoverflow

[levBagryansky]

Closes #2836

---

PR-Codex overview

This PR introduces a new PhTracedEnclosure class to track recursion in dataization process. It also updates EOcage to use PhTracedEnclosure.

Detailed summary

• Added PhTracedEnclosure class to trace recursion in dataization
• Updated EOcage to use PhTracedEnclosure for enclosure handling
• Added a test for exception throwing when recursion occurs

✨ Ask PR-Codex anything about this PR by commenting with /codex {your question}

[levBagryansky]

@maxonfjvipon could you please review

[levBagryansky]

@yegor256 please check

[yegor256]

@volodya-lombrozo please, review this one

[volodya-lombrozo]

@levBagryansky What do you think if we rename this method to attrTraced or tracedAttr?

[levBagryansky]

@volodya-lombrozo attrTraced looks like we return traced attribute, but it is not. Maybe getAttrWithTracingTheCage?

[volodya-lombrozo]

@levBagryansky ok, maybe just attr then? Since the signature is different, we can easily overload this method.
I don't like getAttrWithTracingTheCage because of this.

[levBagryansky]

@volodya-lombrozo this post is about variables. attr is very bad name because you need to read documentation to understand what the method does. Ideally, our code should be understandable without documentation.

[volodya-lombrozo]

@levBagryansky Here you can read about methods:

Here is a simple principle for naming methods in OOP, which I’m trying to follow in my code: it’s a verb if it manipulates, it’s a noun if it builds. That’s it. Nothing in between. Methods like saveFile() or getTitle() don’t fit and must be renamed and refactored.

As for this:

Ideally, our code should be understandable without documentation.

You are absolutely right. attr - is a method which returns attribute, that is. A reader don't need to know "how" it was created. If I need to know "how" - I will read the method body.

[levBagryansky]

@volodya-lombrozo reader don't need to know "how" it traces. If I need to know "how" - I will read the method body. Significant that it traces.
It is obvious that it gets from its signature. Again, to get is responsibility of supplier. Responsibility of the method is to trace that supplier does.
Again, the idea of method is in tracing. It can trace by plural ways, and how it traces is written in implementation. Here the name say you that it not just gets it but traces.

[levBagryansky]

@yegor256 please help. What option is cleaner?

       @Override
        public Attr attr(final String name) {
            return this.getAttrWithTracing(() -> this.enclosure.attr(name));
        }

or

       @Override
        public Attr attr(final String name) {
            return this.attr(() -> this.enclosure.attr(name));
        }

There is also an option to make the the callee generic in order to hoghlight its goal:

        @Override
        public Attr attr(final String name) {
            return this.getItWithTracing(() -> this.enclosure.attr(name));
        }

        private <T> T getItWithTracing(final Supplier<T> supplier) {
            if (EOcage.PhTracedEnclosure.DATAIZING_CAGES.contains(this.cage)) {
                throw new ExFailure("The cage %s is already dataizing", this.cage);
            }
            EOcage.PhTracedEnclosure.DATAIZING_CAGES.add(this.cage);
            final T ret = supplier.get();
            EOcage.PhTracedEnclosure.DATAIZING_CAGES.remove(this.cage);
            return ret;
        }

[levBagryansky]

@yegor256 please check

[yegor256]

@levBagryansky First, using the word get for method names which are not "getters" is an anti-pattern: https://stackoverflow.com/questions/46139501/should-getters-from-domain-classes-not-be-prefixed-with-get-in-ddd-and-why Because of this, getItWithTracing is not a good option.

Second, the ...WithTracing part is an indicator of temporal coupling between statements inside the method, which is also an anti-pattern: https://blog.ploeh.dk/2011/05/24/DesignSmellTemporalCoupling/

A better and more object-oriented design would be to use... guess what, a decorator. You have a Supplier and you want to turn it into another Supplier, which does something when its get() method is called. Make it this way:

@Override
public Attr attr(final String name) {
  return TracingWhileGetting(() -> this.enclosure.attr(name));
}

Here, the TracingWhileGetting is the Supplier that does the "tracing" you need.

[volodya-lombrozo]

@yegor256 Looks like overengineering to me. We use this method only in this class and in the single place. I would agree if we used this TracingWhileGetting in other Phi classes, but we do not do it. Moreover, Tracing and Getting are participles (a participle is a verb form.) But according with the Java specification:

Class names should be nouns, in mixed case with the first letter of each internal word capitalized.

Nouns (not adjectives or verbs.) Maybe it's better to name the method attr? In this case we won't add extra code and will avoid the all convention violations which you mentioned before. What do you think?

[volodya-lombrozo]

@levBagryansky ok, maybe just attr then? Since the signature is different, we can easily overload this method.
I don't like getAttrWithTracingTheCage because of this.

[levBagryansky]

@volodya-lombrozo WDYT about separating private static final class PhTracedEnclosure to separate file?

[volodya-lombrozo]

@volodya-lombrozo WDYT about separating private static final class PhTracedEnclosure to separate file?

@levBagryansky I don't think we need it. Since it is placed inside the class It indicates that PhTracedEnclosure is used only in the single place.

[levBagryansky]

@volodya-lombrozo EOcage becomes too large and ugly as for me.

[volodya-lombrozo]

@levBagryansky I agree then, it makes sense to move it.

[volodya-lombrozo]

@levBagryansky Looks good to me. Thank you.

[levBagryansky]

@yegor256 please check

[levBagryansky]

@yegor256 please check