Skip to main content
1 of 4

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 field names (and without reading methods) that otherTables is meant to hold table aliases. Why not be kind to the code reader (which is often is you, only time later) and call it tableAliases?

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. 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

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. But at the same this class inherited dozens of methods (which can also update it's state) from LinkedList and which doesn't reveal business intent.

Additionally, relationship between ScopedData and LinkedList are not polymorphic, because in order 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 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 communicate business intent.

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)

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.

scopedData.current().otherTables.add(...); 

scopedData.current().columns.add(...);

If you have chosen the route of specific solution focused of the problem at hand, these lines might have look like this:

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 "bags of values" with no encapsulation and useful behavior:

scopedData.current().addAliace(...); 

scopedData.current().addColumn(...);