1
\$\begingroup\$

I have method that fetches sorted and filtered data from Mysql Database, Firstly I had done It by using Java. Now I did it by Mysql but I don't like the way the code looks

I have entity that named Order and It looks like this

public class Order implements Serializable {
    private int orderId;
    private int userId;
    private int carId;
    private String userAddress;
    private String userDestination;
    private double orderCost;
    @JsonSerialize(using = CustomLocalDateTimeJsonSerializer.class)
    private LocalDateTime orderDate;
//getters, setters
}

Method that performs all work

@Override
    public List<Order> findFilSortOrders(Map<String, String> filters, String sortedColumn, boolean descending, int startRow, int rowsPerPage) {
    StringBuilder query = new StringBuilder("SELECT*FROM (SELECT*FROM user_order ");
    ResultSet resultSet = null;
    LinkedHashMap<String,String> filterToQuery;
    List<String> values = new ArrayList<>();
    List<String> keys = new ArrayList<>();
        List<Order> orders = new ArrayList<>();
        if(filters!=null){
            query.append("WHERE ");
            filterToQuery = new LinkedHashMap<>(filters);
            values = new ArrayList<>(filterToQuery.values());
            keys = new ArrayList<>(filterToQuery.keySet());
            keys.forEach(key->
        {
            if(key.equals("orderDate")){
                query.append("date_format(").append(key).append(",'%Y-%m-%d %H:%i') = ? AND ");
            }else{
                query.append(key).append(" = ? AND ");
            }
        });
            query.setLength(query.length()-4);
    }
        query.append(" LIMIT ?, ?) a").append(" ORDER BY ").append(sortedColumn).append(" ").append(descending?"DESC":"ASC;");
    try(Connection connection = MySQLDAOFactory.getConnection();
    PreparedStatement statement = connection.prepareStatement(query.toString())){
       for (int i=0;i<values.size();i++){
           switch (keys.get(i)){
               case "orderId":
               case "userId":
               case "carId":{
                   statement.setInt(i+1, Integer.parseInt(values.get(i)));
                   break;
               }
               case "orderDate":
               case "userAddress":
               case "userDestination":{
                  statement.setString(i+1,values.get(i));
                   break;
               }
               case "orderCost":{
                   statement.setDouble(i+1,Double.parseDouble(values.get(i)));
                   break;
               }
           }
       }
       statement.setInt(values.size()+1,startRow);
       statement.setInt(values.size()+2,rowsPerPage);
        resultSet = statement.executeQuery();
        while(resultSet.next()){
            Order order = new Order();
            order.setCarId(resultSet.getInt("carId"));
            order.setOrderId(resultSet.getInt("orderId"));
            order.setUserId(resultSet.getInt("userId"));
            order.setUserAddress(resultSet.getString("userAddress"));
            order.setUserDestination(resultSet.getString("userDestination"));
            order.setOrderCost(resultSet.getDouble("orderCost"));
            order.setOrderDate(resultSet.getTimestamp("orderDate").toLocalDateTime());
            orders.add(order);
        }
    }catch (SQLException e){
        LOGGER.error(e);
        try {
            if (resultSet != null) {
                resultSet.close();
            }
        } catch (SQLException throwables) {
            LOGGER.error(throwables);
        }
    }
     return orders;
    }

This code works but I dont like it looks If you could help me improve the code, I would be very grateful. Thanks for answering.

\$\endgroup\$
1
  • \$\begingroup\$ The current question title, which states your concerns about the code, applies to too many questions on this site to be useful. The site standard is for the title to simply state the task accomplished by the code. Please see How do I ask a good question?. \$\endgroup\$
    – BCdotWEB
    May 28 at 6:33
0
\$\begingroup\$

First off, the SQL query you are generating probably doesn't do what you want it to do. Because you using a subquery (a SELECT inside a SELECT) you are first limiting the unsorted data, and the sorting it, resulting in returning "random" rows instead of the first rows in sorted order. There is no reason to use a subquery, so instead of

SELECT * FROM (SELECT * FROM user_order WHERE ... LIMIT ?, ?) a ORDER BY ...

use

SELECT * FROM user_order WHERE ... ORDER BY ... LIMIT ?, ?

Then you really should format your code cleanly. The indentation is really bad and you should insert some empty lines between logical code blocks to separate them optically. Also put spaces in lines, specifically around operators, between keywords (if, while etc.) and (, after ; and before {. For example, the line:

for(int i=0;i<values.size();i++){

should look like this:

for (int i = 0; i < values.size(); i++) {

BTW, your editor/IDE should be able to do that for you. Look for a menu item "Format code".


Don't declare variables until you need them. Both resultSet and orders don't need to be declared until the loop at the end:

ResultSet resultSet = statement.executeQuery();
List<Order> orders = new ArrayList<>();
while (resultSet.next()){
\$\endgroup\$
1
  • \$\begingroup\$ Thanks for answer. What about code efficiency?How can I do It by using Java Stream API? \$\endgroup\$ May 28 at 15:21

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.