## Names should Communicate Intent Doesn't it look confusing, `ScopedData` having a generic parameter `ScopedData`? ``` private final ScopedData<ScopeData> scopedData = new ScopedData<>(ScopeData::new); ``` Field names in `ScopeData` are not very clear as well: ``` public final List<Table> tables = new LinkedList<>(); public final List<String> otherTables = new LinkedList<>(); ``` It's impossible to discern just by looking at these fields (*and without reading methods*) that `otherTables` is meant to hold table aliases. Why not be kind to the code reader (*who is often you, just at a later time*) and call it `tableAliases`? <sup>*By the way, `scopedData` field appears at the very bottom of the class `ColumnVisitor` which is not aligned with [code-conventions conventions of the Java language][code_conventions]. It's more intuitive for the code-reader when fields are introduced before they are being used, not the opposite. And there's also a [Sonar rule enforcing this order of class members][sonar_rule]*</sup> ## Is a `Collection` vs Has a `Collection` You implemented `ScopedData` as a subclass of `java.util.LinkedList`. At the same time, you clearly have intention is to give `ScopedData` business behavior though which it's state should be manipulated. However, this class also inherits numerous methods from `LinkedList` which can update its state and doesn't communicate business intent. Additionally, the relationship between `ScopedData` and `LinkedList` are not polymorphic, because in order to leverage behavior that specific to `ScopedData` you have to use its concrete type, not super type. If you declare it as a `List`, `Queue` or `Deque` it would be a `LinkedList` in disguise (*more over, `ScopedData` is not supposed to be used like that*) In this case it makes more sense to use *composition* by declaring a field of type `Deque` (*that's the JDK type you need, when a stack data structure is required*) and exposing only methods that reveal business intent. <sup>By the way, it looks `LinkedList` is your go-to collection. But in most of the scenarios it's not a good choice. `ArrayDeque` is a more performant `Deque` / `Queue`, and `ArrayList` would do better when you need a `List` (*unless you're updating a collection while iterating over it via the means iterator, which is the only case when `LinkedList` shines*)</sup> ## Generic implementations vs Specific implementations Implementations which are tailored to tackle specific problems are usually *simpler* and have *clear intent*. On the other hand, implementing a generic solution requires a firm understanding of use cases to address them properly. When you have only one use case, or you even only started exploring the problem, a generic solution might be not the best time-investment (*because abstractions you came up with at this stage will be suboptimal at best*). Although your code isn't doing much at the moment, you chose the way of generic implementation for `ScopedData`. As a result you have a customized stack, which is meant to work with a dynamically provided type. ```java scopedData.current().otherTables.add(...); scopedData.current().columns.add(...); ``` By design, you're always working with the data on the top of the stack representing query score (i.e. your `current()`). And aren't columns and aliases is something that you'll be dialing with in other use-cases? If you have chosen the route of specific solution, these lines might have look like this: ```java scopedData.addAliace(...); scopedData.addColumn(...); ``` In case if you want to keep `ScopedData` implementation generic, then at the very least you should give domain-specific methods to types it's meant to work with instead of treating them as *"bags of values"* with no encapsulation and useful behavior: ```java scopedData.current().addAliace(...); scopedData.current().addColumn(...); ``` [code_conventions]: https://www.oracle.com/java/technologies/javase/codeconventions-codeexamples.html#182 [sonar_rule]: https://rules.sonarsource.com/java/tag/convention/RSPEC-1213/?search=field