code improvments

code imprs

code improvements

code improvs
This commit is contained in:
LahaLuhem 2023-01-13 17:11:08 +01:00
parent 7053085344
commit d5730455e6
12 changed files with 132 additions and 82 deletions

View file

@ -44,10 +44,13 @@ The application's main class is `Main` class, it runs the entire application. It
### Improvements ### Improvements
Here are some points that I could have improved upon, but did not go through with it because of either time constraints, or because it might be too much for the scope of this assignment. Here are some points that I could have improved upon, but did not go through with it because of either time constraints, or because it might be too much for the scope of this assignment.
+ Ideally all the services, and inner services and APIs being used, should be injected in by dependency injection. In Dart, [get_it](https://pub.dev/packages/get_it), is makes this very easy with a Locator file. But for Java, I was not able to find something that was as easy.
+ The DatabaseService class is tightly coupling the DatabaseApi class. It's better to make the DatabaseApi class a singleton and inject it into the DatabaseService class. + The DatabaseService class is tightly coupling the DatabaseApi class. It's better to make the DatabaseApi class a singleton and inject it into the DatabaseService class.
+ The DatabaseApi class is doing more than just communicating with the database, it's also handling error handling. It would be better to extract the error handling logic into a separate class. + The DatabaseApi class is doing more than just communicating with the database, it's also handling error handling. It would be better to extract the error handling logic into a separate class.
+ Could be rectified by using lots of custom Exceptions in general, but overkill for this scope.
+ The QueryResultsRepository class is tightly coupled to the PersistentResultSetModel class. It would be better to extract the PersistentResultSetModel class into a separate package, and use a factory pattern to create instances of it. + The QueryResultsRepository class is tightly coupled to the PersistentResultSetModel class. It would be better to extract the PersistentResultSetModel class into a separate package, and use a factory pattern to create instances of it.
+ The App class is doing too much. It would be better to extract the logic for each endpoint into separate classes, and use a factory pattern to create instances of them. + The App class is doing too much. It would be better to extract the logic for each endpoint into separate classes, and use a factory pattern to create instances of them.
+ But then again, too much inheritance.
+ The Main class should be refactored to use dependency injection, so that the dependencies can be easily swapped out for testing. + The Main class should be refactored to use dependency injection, so that the dependencies can be easily swapped out for testing.
+ The AppConfigService class should be refactored to use a more robust input validation library, to handle more edge cases. + The AppConfigService class should be refactored to use a more robust input validation library, to handle more edge cases.
+ The ReportGenerationService class should be refactored to support more output formats, such as HTML, CSV and PDF. + The ReportGenerationService class should be refactored to support more output formats, such as HTML, CSV and PDF.

View file

@ -53,16 +53,44 @@ class App {
* generates reports using the `ReportGenerationService` class, and prompts the user for input using the `AppConfigService` class. * generates reports using the `ReportGenerationService` class, and prompts the user for input using the `AppConfigService` class.
*/ */
public void run() { public void run() {
try { executeNoInteractionEndpointPipeline(reportEndpoints);
QueryResultsRepository resultsRepository; executeInteractiveEndpointPipeline(queryEndpoints);
for (final CustomPreparedStatementsRead query : reportEndpoints) { }
resultsRepository = databaseService.executeReadReportsEndpoint(new QueryDTO(query));
reportGenerationService.reportBaseResults(resultsRepository);
}
/**
* This method is responsible for executing a pipeline of read-only queries, using a provided set of
* {@link CustomPreparedStatementsRead} queryEndpoints, without any user interaction.
* The method will iterate over the provided queryEndpoints, and for each query it will:
* Execute the read query using {@link DatabaseService#getReadReportsEndpoint(QueryDTO)}
* Store the results in a new {@link QueryResultsRepository}
* Generate a report for the stored results using {@link ReportGenerationService#reportBaseResults(QueryResultsRepository)}
* @param queryEndpoints an array of {@link CustomPreparedStatementsRead} containing the queries to be executed.
* @throws RuntimeException when there is a SQLException thrown by {@link DatabaseService#getReadReportsEndpoint(QueryDTO)}
*/
private void executeNoInteractionEndpointPipeline(CustomPreparedStatementsRead[] queryEndpoints) {
QueryResultsRepository resultsRepository;
for (final CustomPreparedStatementsRead query : queryEndpoints) {
try {
resultsRepository = databaseService.getReadReportsEndpoint(new QueryDTO(query));
} catch (SQLException e) {
throw new RuntimeException(e);
}
reportGenerationService.reportBaseResults(resultsRepository);
}
}
/**
* Executes a series of read queries specified in the {@code queryEndpoints} parameter, and prompts the user for input of country name and date.
* The results of the queries are passed to reportGenerationService to be reported.
* @param queryEndpoints the endpoints to execute in the pipeline
* @throws RuntimeException if a SQLException occurs when getting the read reports endpoint
*/
private void executeInteractiveEndpointPipeline(CustomPreparedStatementsRead[] queryEndpoints) {
QueryResultsRepository resultsRepository;
try {
for(final CustomPreparedStatementsRead query: queryEndpoints) { for(final CustomPreparedStatementsRead query: queryEndpoints) {
appConfigService.promptAndSetUserArguments(); appConfigService.promptAndSetUserArguments();
resultsRepository = databaseService.executeReadReportsEndpoint(new QueryDTO(query, new Object[]{appConfigService.countryName, appConfigService.date,})); resultsRepository = databaseService.getReadReportsEndpoint(new QueryDTO(query, new Object[]{appConfigService.getCountryName(), appConfigService.getDate(),}));
reportGenerationService.reportBaseResults(resultsRepository); reportGenerationService.reportBaseResults(resultsRepository);
} }

View file

@ -5,24 +5,22 @@ import org.postgresql.util.PSQLException;
import java.sql.*; import java.sql.*;
import java.util.Date; import java.util.Date;
import java.util.MissingFormatArgumentException;
/** /**
* The `DatabaseApi` class provides a way to communicate with the database to perform read operations and return the results. * The `DatabaseApi` class is a DAO that provides a way to communicate with the database to perform read operations and return the results.
* It contains a single method `performReadQuery()` that takes a `QueryDTO` and a `Connection` object as parameters and * It contains a single method `performReadQuery()` that takes a `QueryDTO` and a `Connection` object as parameters and
* returns a `PreparedStatement` object. It throws `SQLException` in case of any SQL error or `MissingFormatArgumentException` * returns a `PreparedStatement` object. It throws `SQLException` in case of any SQL error or `IllegalArgumentException`
* in case of any missing argument or extra argument. * in case of any missing argument or extra argument.
* * <p>
* The class can be abstracted and implemented into constituent action-specific APIs when there is a need for more than a single READ action. * The class can be abstracted and implemented into constituent action-specific APIs when there is a need for more than a single READ action.
*/ */
public class DatabaseApi { public class DatabaseApi {
/** /**
* Creates and executes a PreparedStatement, given the QueryDTO. * Creates and executes a PreparedStatement, given the QueryDTO.
* @param queryDTO * @param queryDTO The query in the form of the queryDTO
* @param connection * @param connection SQL server connection
* @return * @return The executed prepared statement, containing the results of the query
* @throws MissingFormatArgumentException in case of any missing argument or extra argument. * @throws IllegalArgumentException in case of any missing argument or extra argument.
* @throws SQLException
*/ */
public PreparedStatement performReadQuery(QueryDTO queryDTO, Connection connection) throws SQLException { public PreparedStatement performReadQuery(QueryDTO queryDTO, Connection connection) throws SQLException {
// Set template // Set template
@ -43,12 +41,15 @@ public class DatabaseApi {
statement.execute(); statement.execute();
} catch (PSQLException e) { } catch (PSQLException e) {
if (e.getMessage().startsWith("No value specified for parameter")) throw new MissingFormatArgumentException("The templates present in the query, were not correctly mapped to their replacement variables."); if (e.getMessage().startsWith("No value specified for parameter"))
else if (e.getMessage().startsWith("The column index is out of range:")) throw new MissingFormatArgumentException("No replacement variables needed, but were they were still supplied."); throw new IllegalArgumentException("The templates present in the query, were not correctly mapped to their replacement variables.");
else if (e.getMessage().startsWith("The column index is out of range:")) {
throw new IllegalArgumentException("Are you trying to supply replacement variables, when they are not needed?");
}
throw e; throw e;
} }
return statement; return statement;
} }
} }

View file

@ -2,6 +2,6 @@ package constants;
import java.text.SimpleDateFormat; import java.text.SimpleDateFormat;
public class ConstFormatters{ public class Formatters {
final public static SimpleDateFormat APPLICATION_DATE_FORMATTER = new SimpleDateFormat("dd-MM-yyyy"); final public static SimpleDateFormat APPLICATION_DATE_FORMATTER = new SimpleDateFormat("dd-MM-yyyy");
} }

View file

@ -5,7 +5,7 @@ import data.enums.CustomPreparedStatementsRead;
/** /**
* Container for storing SQL statements to be executed, along with their aruments. * Container for storing SQL statements to be executed, along with their aruments.
* @param statement an instance of `CustomPreparedStatementsRead` enum which contains the statement template to be executed. * @param statement an instance of `CustomPreparedStatementsRead` enum which contains the statement template to be executed.
* @param templateArgsn an array of `Object` representing the arguments to be used in the statement template. * @param templateArgs an array of `Object` representing the arguments to be used in the statement template.
*/ */
public record QueryDTO(CustomPreparedStatementsRead statement, Object[] templateArgs) { public record QueryDTO(CustomPreparedStatementsRead statement, Object[] templateArgs) {
/** /**

View file

@ -70,7 +70,7 @@ public enum CustomPreparedStatementsRead {
public final String statementTemplate; public final String statementTemplate;
public final String description; public final String description;
private CustomPreparedStatementsRead(String statementTemplate, String description) { CustomPreparedStatementsRead(String statementTemplate, String description) {
this.statementTemplate = statementTemplate; this.statementTemplate = statementTemplate;
this.description = description; this.description = description;
} }

View file

@ -30,17 +30,16 @@ public class PersistentResultSetModel {
// Retrieving rest of the results // Retrieving rest of the results
final List<ResultRowModel> rows = new ArrayList<>(); final List<ResultRowModel> rows = new ArrayList<>();
final Object[] rowElements = new Object[columnCount];
while (resultSet.next()) { while (resultSet.next()) {
for (int columnIndex = 0; columnIndex < rowElements.length; columnIndex++) final Object[] rowElements = new Object[columnCount];
for (int columnIndex = 0; columnIndex < columnCount; columnIndex++)
rowElements[columnIndex] = resultSet.getObject(columnIndex+1); rowElements[columnIndex] = resultSet.getObject(columnIndex+1);
rows.add(new ResultRowModel(rowElements.clone())); rows.add(new ResultRowModel(rowElements));
} }
resultRowEntries = new ResultRowModel[rows.size()]; resultRowEntries = new ResultRowModel[rows.size()];
rows.toArray(resultRowEntries); rows.toArray(resultRowEntries);
} }
/** /**
@ -51,9 +50,6 @@ public class PersistentResultSetModel {
* Represents column names that appear in the result set. * Represents column names that appear in the result set.
*/ */
/**
* It returns the string representation of the result set in tabular format, showing the column names and rowElements.
*/
final public String[] columnNames; final public String[] columnNames;
/** /**

View file

@ -1,38 +1,40 @@
package data.repos; package data.repos;
import data.models.PersistentResultSetModel; import data.models.PersistentResultSetModel;
import java.sql.ResultSet; import java.sql.ResultSet;
import java.sql.SQLException; import java.sql.SQLException;
import java.util.ArrayList; import java.util.ArrayList;
import java.util.List;
/** /**
* Provides a way to store and persist the results of read queries. * Provides a way to store and persist the results of read queries.
* It contains an `ArrayList` of `PersistentResultSetModel` called `persistentResults` and a `String` called `queryDescription`. * It contains a list of `PersistentResultSetModel` instances called `persistentResults` and a `String` called `queryDescription`.
* The class provides a constructor that takes a `queryDescription` as a parameter and an `addResult()` method * The class provides a constructor that takes a `queryDescription` as a parameter and an `addResult()` method
* that takes a `ResultSet` as a parameter and adds it to the `persistentResults` list. * that takes a `ResultSet` as a parameter and adds it to the `persistentResults` list.
* The class also provides a `toString()` method that returns the string representation of the stored results and their description. * The class also provides a `toString()` method that returns the string representation of the stored results and their description.
*/ */
public class QueryResultsRepository { public class QueryResultsRepository {
/**
* Store for the results
*/
private final List<PersistentResultSetModel> persistentResults = new ArrayList<>();
/**
* Represents a description of the query that was executed.
*/
private final String queryDescription;
public QueryResultsRepository(String queryDescription) { public QueryResultsRepository(String queryDescription) {
this.queryDescription = queryDescription; this.queryDescription = queryDescription;
} }
/**
* Store for the results
*/
final private ArrayList<PersistentResultSetModel> persistentResults = new ArrayList<>();
/**
* Represents a description of the query that was executed.
*/
final private String queryDescription;
/** /**
* Creates a PersistentResultSetModel instance from given ResultSet, and adds it to the persistentResults list * Creates a PersistentResultSetModel instance from given ResultSet, and adds it to the persistentResults list
* @param resultSet *
* @throws SQLException * @param resultSet the resultSet to be added to the persistentResults list
* @throws SQLException if an error occurs while creating the PersistentResultSetModel instance
*/ */
public void addResult(ResultSet resultSet) throws SQLException { public void addResult(ResultSet resultSet) throws SQLException {
persistentResults.add(new PersistentResultSetModel(resultSet)); persistentResults.add(new PersistentResultSetModel(resultSet));
@ -40,11 +42,11 @@ public class QueryResultsRepository {
@Override @Override
public String toString() { public String toString() {
StringBuilder templateBuilder = new StringBuilder("\n\nDESCRIPTION: ").append(queryDescription); StringBuilder templateBuilder = new StringBuilder("\n\nQUERY DESCRIPTION: ").append(queryDescription);
for (final PersistentResultSetModel result : persistentResults) { for (final PersistentResultSetModel result : persistentResults) {
templateBuilder.append("\n").append(result).append("\n>"); templateBuilder.append("\n").append(result).append("\n>");
} }
return templateBuilder.deleteCharAt(templateBuilder.length()-1).append("\n\n").toString(); return templateBuilder.deleteCharAt(templateBuilder.length() - 1).append("\n\n").toString();
} }
} }

View file

@ -1,7 +1,7 @@
package services; package services;
import utils.Formatters; import utils.Formatters;
import utils.ConstValues; import utils.ConstText;
import java.text.ParseException; import java.text.ParseException;
@ -12,37 +12,39 @@ import java.util.Scanner;
* Prompts and stores the configuration that the app needs to be run with. * Prompts and stores the configuration that the app needs to be run with.
*/ */
public class AppConfigService { public class AppConfigService {
String countryName; private String countryName;
Date date; public String getCountryName() {return countryName;}
private Date date;
public Date getDate() {return date;}
/** /**
* Prompts the user to input the country name and date, and stores the input. * Prompts the user to input the country name and date, and stores the input.
* It will keep prompting the user until the date is in the correct format. * It will keep prompting the user until the date is in the correct format.
*/ */
public void promptAndSetUserArguments() { public void promptAndSetUserArguments() {
final Scanner scanner = new Scanner(System.in); // Closing even on an uncaught exception
System.out.println(ConstValues.inputCountryName); try (Scanner scanner = new Scanner(System.in)) {
countryName = scanner.nextLine(); System.out.println(ConstText.inputCountryName);
System.out.println(); countryName = scanner.nextLine();
System.out.println();
System.out.println(ConstValues.inputDate); System.out.println(ConstText.inputDate);
while (true) { while (true) {
try { try {
date = Formatters.APPLICATION_DATE_FORMATTER.parse(scanner.nextLine()); date = Formatters.APPLICATION_DATE_FORMATTER.parse(scanner.nextLine());
// Always using exact current date. Otherwise, different day possible at the start of the run. // Always using exact current date. Otherwise, different day possible at the start of the run.
if (date.after(new Date())) { if (date.after(new Date())) {
System.out.println("There is no data available for the future yet... Try again:"); System.out.println(ConstText.dateInFuture);
continue; continue;
}
// Not doing anymore month/day validations.
break;
} catch (ParseException e) {
System.out.printf(ConstText.unableParseDate, Formatters.APPLICATION_DATE_FORMATTER.toPattern());
} }
// Not doing anymore month/day validations.
break;
} catch (ParseException e) {
System.out.printf("Unable to parse date. Should be in format '%s'.\nTry again:%n", Formatters.APPLICATION_DATE_FORMATTER.toPattern());
} }
} }
scanner.close();
} }
} }

View file

@ -3,6 +3,7 @@ package services;
import apis.DatabaseApi; import apis.DatabaseApi;
import data.dtos.QueryDTO; import data.dtos.QueryDTO;
import data.repos.QueryResultsRepository; import data.repos.QueryResultsRepository;
import utils.ConstText;
import java.sql.*; import java.sql.*;
@ -30,7 +31,7 @@ public class DatabaseService {
* @param queryDTO The query with its arguments. * @param queryDTO The query with its arguments.
* @return Persisting (non-lazy) results from executing the query. * @return Persisting (non-lazy) results from executing the query.
*/ */
public QueryResultsRepository executeReadReportsEndpoint(QueryDTO queryDTO) throws SQLException { public QueryResultsRepository getReadReportsEndpoint(QueryDTO queryDTO) throws SQLException {
final PreparedStatement resultStatement = databaseApi.performReadQuery(queryDTO, connection); final PreparedStatement resultStatement = databaseApi.performReadQuery(queryDTO, connection);
final QueryResultsRepository resultSets = new QueryResultsRepository(queryDTO.statement().description); final QueryResultsRepository resultSets = new QueryResultsRepository(queryDTO.statement().description);
@ -52,9 +53,9 @@ public class DatabaseService {
Connection connection = null; Connection connection = null;
try { try {
connection = DriverManager.getConnection(url, user, password); connection = DriverManager.getConnection(url, user, password);
System.out.println("Connected to the PostgreSQL server successfully."); System.out.println(ConstText.successServerConnection);
} catch (SQLException e) { } catch (SQLException e) {
System.out.println("Unable to connect to PostgreSQL server."); System.out.println(ConstText.failureServerConnection);
throw new RuntimeException(); throw new RuntimeException();
} }

24
src/utils/ConstText.java Normal file
View file

@ -0,0 +1,24 @@
package utils;
/**
* Contains string constants used in the application for the purpose of user I/O.
* <p>
* Effectively a simpler standalone for l10n localization.
*/
public class ConstText {
//D
public static final String dateInFuture = "There is no data available for the future yet... Try again:";
//F
public static final String failureServerConnection = "Unable to connect to PostgreSQL server.";
//I
public static final String inputCountryName = "Enter country: ";
public static final String inputDate = "Enter date: ";
//S
public static final String successServerConnection = "Connected to the PostgreSQL server successfully.";
//U
public static final String unableParseDate = "Unable to parse date. Should be in format '%s'.\nTry again:%n";
}

View file

@ -1,7 +0,0 @@
package utils;
public class ConstValues {
//I
final public static String inputCountryName = "Enter country: ";
final public static String inputDate = "Enter date: ";
}