Using decorator and strategy pattern for dynamically generating SQL queries

Question

This part of the application design deals with generating sql query strings dynamically This is the entire detailed architecture of the application

This is a link to my application that actually runs on the design specified in the image.

The idea behind the image is that the SubjectInfoViewer behaves as the context in the strategy pattern, and the ViewingQueryComponent behaves as the strategy interface, as well as the component class in the decorator pattern, and its implementing classes except the Filter class are the different strategies. These strategies return an SQL string specific to their description, which can be decorated using the Filter class, which is the decorator, in the decorator pattern. The classes implementing the filter class just append the where clause of the SQL string produced by the strategies and push the parameters involved in the where clause in a parameter stack, so they can finally by used by a parameterized SQL statement involving the whole "stitched" SQL string.

For some reason, this doesn't seem to be the right approach to tackle this particular usecase. Please suggest the best practices used in this situation.

The reason this doesn't seem to be the right approach is that there are a lot of tables being joined in the base queries, just for the convenience of writing where clauses in the Filters.

I really would like to reduce the accidental complexity of having to push the parameters used in where clauses into the paramStack for using them in a parameterized query and having to call all the filters only for some filters to do nothing but by pass the call to another filter without doing anything useful.

By the way, the repeated code in the base queries can be easily abstracted.. Also, better decoupling can be ensured by isolating the UserInterface reference to the SubjectInfoViewer so that it passes the parameters it receives from the getters of the UserInterace to the Filters, instead of having the Filters get them from the UserInterface.

SubjectInfoViewer.java

public class SubjectInfoViewer {
 private ViewingQueryComponent baseQuery;
 private ViewingQueryComponent vqc;
 private String viewingQuery;
 private Stack<String> viewParams=new Stack<>();
 private UserInterface ui;
 public void changeBaseQuery(ViewingQueryComponent vqc){
 baseQuery=vqc;
 }
 public void stitch(){
 vqc=new COFilter(ui,viewParams,
 new USNFilter(ui,viewParams,
 new DifficultyFilter(ui,viewParams,
 new SectionFilter(ui,viewParams,
 new DateFilter(ui,viewParams,
 new ModuleFilter(ui,viewParams,
 new SubjectFilter(ui,viewParams,
 baseQuery)))))));
 viewingQuery=vqc.stitch()+" GROUP BY STATUS.Topic_Name,QUESTION_BANK.Question_Statement"; 
 }
 public SubjectInfoViewer(UserInterface ui) {
 this.ui=ui;
 baseQuery=new DefaultViewingQuery();
 }
 public DefaultTableModel getTableModel() {
 return new DBGateway().getSubjectDetails(viewingQuery,viewParams);
 }
} 

ViewingQueryComponent.java

public interface ViewingQueryComponent {
String stitch();}

DefaultViewingQuery.java

public class DefaultViewingQuery implements ViewingQueryComponent {
private String sql= 
 "SELECT "
 + "TOPICS.Topic_Name AS \"Topic Name\", "
 + "TOPICS.Textbook_Name AS \"Textbook Name\", "
 + "TOPICS.Page_Number AS \"Page Number\", "
 + "MADE_FROM.Question_Statement AS \"Question Statement\", "
 + "QUESTION_BANK.Total_Marks AS \"Total Marks\", "
 + "ROUND((COUNT(DISTINCT STATUS.USN)/(SELECT SUM(STUDENT.USN) FROM STUDENT))*100,2) AS \"Total Students (%)\" "
 + "FROM "
 + "STATUS, "
 + "TEXTBOOK, "
 + "SUBJECT, "
 + "STUDENT, "
 + "DISTRIBUTE, "
 + "TOPICS LEFT JOIN (MADE_FROM,QUESTION_BANK) ON TOPICS.Topic_Name = MADE_FROM.Topic_Name AND QUESTION_BANK.Question_Statement=MADE_FROM.Question_Statement "
 + "WHERE "
 + "DISTRIBUTE.Topic_Name=TOPICS.Topic_Name and "
 + "TEXTBOOK.Textbook_Name=TOPICS.Textbook_Name and "
 + "STATUS.Topic_Name=TOPICS.Topic_Name and "
 + "STATUS.USN=STUDENT.USN ";
@Override
public String stitch() {
 return sql;
}}

SectionViewingQuery.java

public class SectionViewingQuery implements ViewingQueryComponent{
private String sql;
public SectionViewingQuery(UserInterface ui){
 sql= "SELECT "
 + "TOPICS.Topic_Name AS \"Topic Name\", "
 + "TOPICS.Textbook_Name AS \"Textbook Name\", "
 + "TOPICS.Page_Number AS \"Page Number\", "
 + "MADE_FROM.Question_Statement AS \"Question Statement\", "
 + "QUESTION_BANK.Total_Marks AS \"Total Marks\", "
 + "(COUNT(DISTINCT STATUS.USN)/(SELECT SUM(STUDENT.USN) FROM STUDENT WHERE STUDENT.Section=\""+ui.getSection()+"\"))*100 AS \"Total Students (%)\" "
 + "FROM "
 + "STATUS, "
 + "TEXTBOOK, "
 + "SUBJECT, "
 + "STUDENT, "
 + "DISTRIBUTE, "
 + "TOPICS LEFT JOIN (MADE_FROM,QUESTION_BANK) ON TOPICS.Topic_Name = MADE_FROM.Topic_Name AND QUESTION_BANK.Question_Statement=MADE_FROM.Question_Statement "
 + "WHERE "
 + "DISTRIBUTE.Topic_Name=TOPICS.Topic_Name and "
 + "TEXTBOOK.Textbook_Name=TOPICS.Textbook_Name and "
 + "STATUS.Topic_Name=TOPICS.Topic_Name and "
 + "STATUS.USN=STUDENT.USN ";
}
@Override
public String stitch() {
 return sql;
}}

Filter.java

public abstract class Filter implements ViewingQueryComponent {
private ViewingQueryComponent vqc;
protected Stack<String> paramStack;
protected String sql="";
abstract boolean setSql();
Filter(ViewingQueryComponent vqc,Stack<String> paramStack){
 this.paramStack=paramStack;
 this.vqc=vqc;
}
@Override
public String stitch(){
 if(setSql())
 return vqc.stitch()+" and "+sql;
 return vqc.stitch()+sql;
}}

SubjectFilter.java

public class SubjectFilter extends Filter{
String subject;
public SubjectFilter(UserInterface ui,Stack<String> paramStack,ViewingQueryComponent vqc) {
 super(vqc,paramStack);
 subject=ui.getSubject();
}
@Override
boolean setSql() {
 if(!CheckHelper.checkEmpty(subject)){
 sql=" TEXTBOOK.Subject_Name=? ";
 paramStack.push(subject);
 return true;
 }return false;
}}

• gervais.b: Hello @Muhammed. This is the kind of questions that you should ask on stackoverflow, not codereview (because there is not code to review). But you should explain why "this doesn't seem to be the right approach"
• Muhammad Luqman: Hi @gervais.b I have updated the question to reflect the suggestions made. Thank you. And regarding the explanation of why this doesn't seem to be the right approach, It is because of the awkwardness of pushing parameters used in the where clause into a stack to be later used in the parameterized sql query.
• Muhammad Luqman: let me know if anyone needs more code to help them better review.

Answer 1

• You've followed the Java naming conventions, but several functions are named within the programming solution domain rather than the business problem domain. Stick to the problem domain when naming.
• Some bits of the code are difficult to follow, the stitch() method for example, with all the nested function calls to complete the parameters, you can achieve the same level of flexibility but with a large increase in clarity by using the builder pattern. Name the methods using the field names from the problem domain.
• Concatenating strings to construct SQL queries looks simple but it can quickly become unmanageable as you've discovered. It is also extremely dangerous for security being a risk of SQL injections attacks. Do not do it. Instead use PreparedStatements which works well with the Builder Pattern.

https://docs.oracle.com/javase/tutorial/jdbc/basics/prepared.html

• Muhammad Luqman: Regarding the first point, isn't it inevitable to have some functions or classes represent the solution instead of the problem domain ?
• Muhammad Luqman: As for the second point, thank you for the suggestion. I'll look into it seeking to find a builder solution that follows the open closed principle, like the current solution does. As for the third point, I have actually used PreparedStatements in the DBGateway. I'm sorry that I haven't made it obvious. but if you look closely at the last snippet's use of ? you might get a clue. But again, i'll try to see how builder pattern fits here.

Answer 2

I have made some names a bit more obvious with respect to their purpose.

The inconvenience of using a parameter stack can be overcome, by creating a class ViewingQuery which encapsulates the sql query as well as its parameters.

This could be instantiated as an immutable object, which gets transformed by the ViewingQueryComponents' composeViewingQuery() method.

The guard condition can be implemented within the ViewingQuery class.

An object of the ViewingQuery class named compositeViewingQuery shall be composed in the SubjectInfoViewer class.

The composeViewingQuery() in the SubjectInfoViewer shall transform the ViewingQuery and assign it to compositeViewingQuery

Although this approach provides nice abstraction and better adheres to the single responsibility principle, it has an accidental complexity built into it.

The accidental complexity is that, getViewTableModel() is dependent on the state changed by composeViewingQuery(). Hence the sequence of invocation of these methods becomes important.

One way to overcome this accidental complexity, is by making the composeViewingQuery() method in the SubjectInfoViewer private, and calling it from getViewTableModel() and removing the attribute compositeViewingQuery at the cost of violating single responsibility principle. This move would also make testing hard.

Another way to overcome this accidental complexity, is by having the composeViewingQuery() method of SubjectInfoViewer return the compositeViewingQuery. Then remove the getViewTableModel method. This way, the client directly calls the getSubjectDetails method of DBGateway by passing the value returned from composeViewingQuery. This may be the best approach.

Also, the current solution can be improved by passing an instance of ViewingQuery as a parameter for composeViewingQuery() in order to improve readability.

The following is the refactored code:

ViewingQuery.java

public class ViewingQuery {
 
 private final List<String> parameterList;
 private final String baseQuery;
 private final String queryFilters;
 
 ViewingQuery(){
 parameterList=new ArrayList<>();
 baseQuery="";
 queryFilters="";
 }
 ViewingQuery(List parameterList,String queryFilter,String baseQuery){
 this.parameterList=parameterList;
 this.baseQuery=baseQuery;
 this.queryFilters=queryFilter;
 }
 
 public ViewingQuery withFilter(String queryFilter,String ... parameters){
 if(!CheckHelper.checkEmpty(parameters)){
 List<String> newParameterList=getNewParameterList(parameters);
 return new ViewingQuery(newParameterList,this.queryFilters+" and "+queryFilter,this.baseQuery);
 }
 return this;
 }
 private List<String> getNewParameterList(String[] parameters) {
 List<String> newParameterList=new ArrayList<>();
 newParameterList.addAll(this.parameterList);
 newParameterList.addAll(Arrays.asList(parameters));
 return newParameterList;
 }
 public ViewingQuery withBaseQuery(String baseQuery,String ... parameters){
 List<String> newParameterList=getNewParameterList(parameters);
 return new ViewingQuery(newParameterList,this.queryFilters,baseQuery);
 } 
 public String getQuery(){
 if(CheckHelper.checkEmpty(baseQuery))
 return "";
 return baseQuery+queryFilters+" GROUP BY STATUS.Topic_Name,QUESTION_BANK.Question_Statement";
 }
 public List<String> getParameterList(){
 return parameterList;
 }
}

SubjectInfoViewer.java

public class SubjectInfoViewer {
 private ViewingQueryComponent baseQueryComponent;
 private final UserInterface ui;
 
 public SubjectInfoViewer(UserInterface ui) {
 this.ui=ui;
 baseQueryComponent=new DefaultViewingQuery();
 } 
 
 public void changeBaseQueryComponent(ViewingQueryComponent baseQueryComponent){
 this.baseQueryComponent=baseQueryComponent;
 }
 public ViewingQuery composeViewingQuery(){
 return new COFilter(ui.getCO(), 
 new USNFilter(ui.getUSN(), 
 new DifficultyFilter(ui.getDifficulty(), 
 new SectionFilter(ui.getSection(), 
 new DateFilter(ui.getInitialDate(),ui.getFinalDate(), 
 new ModuleFilter(ui.getModule(), 
 new SubjectFilter(ui.getSubject(), 
 baseQueryComponent))))))).composeViewingQuery(); 
 }
}

ViewingQueryComponent.java

public interface ViewingQueryComponent {
 
 ViewingQuery composeViewingQuery();
}

DefaultViewingQuery.java

public class DefaultViewingQuery implements ViewingQueryComponent {
 private String sql= 
 "SELECT "
 + "TOPICS.Topic_Name AS \"Topic Name\", "
 + "TOPICS.Textbook_Name AS \"Textbook Name\", "
 + "TOPICS.Page_Number AS \"Page Number\", "
 + "MADE_FROM.Question_Statement AS \"Question Statement\", "
 + "QUESTION_BANK.Total_Marks AS \"Total Marks\", "
 + "ROUND((COUNT(DISTINCT STATUS.USN)/(SELECT SUM(STUDENT.USN) FROM STUDENT))*100,2) AS \"Total Students (%)\" "
 + "FROM "
 + "STATUS, "
 + "TEXTBOOK, "
 + "SUBJECT, "
 + "STUDENT, "
 + "DISTRIBUTE, "
 + "TOPICS LEFT JOIN (MADE_FROM,QUESTION_BANK) ON TOPICS.Topic_Name = MADE_FROM.Topic_Name AND QUESTION_BANK.Question_Statement=MADE_FROM.Question_Statement "
 + "WHERE "
 + "DISTRIBUTE.Topic_Name=TOPICS.Topic_Name and "
 + "TEXTBOOK.Textbook_Name=TOPICS.Textbook_Name and "
 + "STATUS.Topic_Name=TOPICS.Topic_Name and "
 + "STATUS.USN=STUDENT.USN ";
 @Override
 public ViewingQuery composeViewingQuery() {
 return new ViewingQuery().withBaseQuery(sql);
 } 
}

SectionViewingQuery.java

public class SectionViewingQuery implements ViewingQueryComponent{
 private final String sql;
 private final String section;
 public SectionViewingQuery(String section){
 this.section=section;
 sql= "SELECT "
 + "TOPICS.Topic_Name AS \"Topic Name\", "
 + "TOPICS.Textbook_Name AS \"Textbook Name\", "
 + "TOPICS.Page_Number AS \"Page Number\", "
 + "MADE_FROM.Question_Statement AS \"Question Statement\", "
 + "QUESTION_BANK.Total_Marks AS \"Total Marks\", "
 + "(COUNT(DISTINCT STATUS.USN)/(SELECT SUM(STUDENT.USN) FROM STUDENT WHERE STUDENT.Section=?))*100 AS \"Total Students (%)\" "
 + "FROM "
 + "STATUS, "
 + "TEXTBOOK, "
 + "SUBJECT, "
 + "STUDENT, "
 + "DISTRIBUTE, "
 + "TOPICS LEFT JOIN (MADE_FROM,QUESTION_BANK) ON TOPICS.Topic_Name = MADE_FROM.Topic_Name AND QUESTION_BANK.Question_Statement=MADE_FROM.Question_Statement "
 + "WHERE "
 + "DISTRIBUTE.Topic_Name=TOPICS.Topic_Name and "
 + "TEXTBOOK.Textbook_Name=TOPICS.Textbook_Name and "
 + "STATUS.Topic_Name=TOPICS.Topic_Name and "
 + "STATUS.USN=STUDENT.USN ";
 }
 @Override
 public ViewingQuery composeViewingQuery() {
 return new ViewingQuery().withBaseQuery(sql,section);
 } 
}

Filter.java

public abstract class Filter implements ViewingQueryComponent {
 protected final ViewingQueryComponent vqc; 
 
 Filter(ViewingQueryComponent vqc){ 
 this.vqc=vqc;
 }
}

SubjectFilter.java

public class SubjectFilter extends Filter{
 private final String subject;
 private final String sql;
 public SubjectFilter(String subject,ViewingQueryComponent vqc) {
 super(vqc);
 sql="TEXTBOOK.Subject_Name=?";
 this.subject=subject;
 }
 @Override
 public ViewingQuery composeViewingQuery() {
 return vqc.composeViewingQuery().withFilter(sql, subject);
 }
}

There's an inherent limitation in using the decorator pattern like this. The limitation is that you cannot selectively remove filters from existing filter chains dynamically, hopefully there might be an ideal solution that preserves the functional nature of the solution while over coming this limitation.

Perhaps the intercepting filter design pattern is more appropriate here instead of the decorator design pattern. Make the SubjectInfoViewer the FilterManager and implement, for every Filter the hashCode and equals methods as shown here as the state of the Filters become irrelevant for every composeViewingQuery message passed from the ui so they can be easily removed in the FilterChain, if implemented as a Set.

But this pattern although will provide more flexibility than the decorator, it will need to pass unneeded parameters into the constructors of the filters just to remove the filters from the FilterChain. And this solution will possibly be feasible at the expense of sacrificing the declarative nature of the decorator solution