Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

added solution to review #1

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
23 changes: 23 additions & 0 deletions src/main/java/mate/jdbc/Main.java
Original file line number Diff line number Diff line change
@@ -1,7 +1,30 @@
package mate.jdbc;

import mate.jdbc.dao.ManufacturerDao;
import mate.jdbc.dao.ManufacturerDaoImpl;
import mate.jdbc.model.Manufacturer;

public class Main {

public static void main(String[] args) {
ManufacturerDao manufacturerDao = new ManufacturerDaoImpl();
Manufacturer manufacturer1 = new Manufacturer(null, "Lincoln", "USA");
Manufacturer manufacturer2 = new Manufacturer(null, "Ford", "USA");
Manufacturer manufacturer3 = new Manufacturer(null, "Audi", "Germany");
Manufacturer manufacturer4 = new Manufacturer(null, "BMW", "Germany");
Comment on lines +11 to +14
Copy link
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

  1. Use an informative variable names
  2. Use a constructor with 2 parameters: name, country

manufacturerDao.create(manufacturer1);
manufacturerDao.create(manufacturer2);
manufacturerDao.create(manufacturer3);
manufacturerDao.create(manufacturer4);

manufacturerDao.delete(16L);
manufacturerDao.delete(22L);
manufacturerDao.delete(24L);
manufacturerDao.delete(27L);

System.out.println(manufacturerDao.get(23L));
manufacturerDao.update(new Manufacturer(23L, "Volkswagen", "Germany"));
System.out.println(manufacturerDao.get(23L));
Comment on lines +15 to +27
Copy link
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

  1. Override method toString() in the Manufacturer class
  2. Check if records were inserted by calling methods get() and getAll()
  3. Check if record(s) were deleted by calling methods delete() and get()


}
}
16 changes: 16 additions & 0 deletions src/main/java/mate/jdbc/dao/ManufacturerDao.java
Original file line number Diff line number Diff line change
@@ -0,0 +1,16 @@
package mate.jdbc.dao;

import java.util.List;
import mate.jdbc.model.Manufacturer;

public interface ManufacturerDao {
Manufacturer create(Manufacturer manufacturer);

Manufacturer get(Long id);

List<Manufacturer> getAll() throws RuntimeException;

Manufacturer update(Manufacturer manufacturer);

void delete(Long id);
}
111 changes: 111 additions & 0 deletions src/main/java/mate/jdbc/dao/ManufacturerDaoImpl.java
Original file line number Diff line number Diff line change
@@ -0,0 +1,111 @@
package mate.jdbc.dao;

import java.sql.Connection;
import java.sql.PreparedStatement;
import java.sql.ResultSet;
import java.sql.SQLException;
import java.sql.Statement;
import java.util.ArrayList;
import java.util.List;
import mate.jdbc.lib.Dao;
import mate.jdbc.model.Manufacturer;
import mate.jdbc.util.ConnectionUtil;

@Dao
public class ManufacturerDaoImpl implements ManufacturerDao {
@Override
public Manufacturer create(Manufacturer manufacturer) {
try (Connection connection = ConnectionUtil.getConnection();
PreparedStatement createStatement =
connection.prepareStatement(
"INSERT INTO manufacturers(name, country) values(?,?);",
Statement.RETURN_GENERATED_KEYS)) {
Comment on lines +19 to +22
Copy link
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

  1. You have to close the PreparedStatement after you’re done with it and before you create a new one on the same connection. Use try with resources for that. The same mistake in other methods
  2. Remember about SQL style: use uppercase for SQL keywords in your queries. The same in method delete()
  3. Let’s save each query in a separate variable or in a constant (event better) The same in getAll() method

createStatement.setString(1, manufacturer.getName());
createStatement.setString(2, manufacturer.getCountry());
ResultSet generatedKeys = createStatement.getGeneratedKeys();
Copy link
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You access the generated key (id) before executing the insert query

if (generatedKeys.next()) {
Long id = generatedKeys.getLong(1);
Copy link
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Use a generatedKeys.getObject() instead, because getLong() return '0' if data is absent. The same in method get()

manufacturer.setId(id);
}
createStatement.executeUpdate();
} catch (SQLException e) {
throw new RuntimeException("Error");
Copy link
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

  1. According to the task you need to create a custom exception DataProcessingException and you should rethrow this exception in catch block. The same in other methods
  2. Use informative messages for exceptions

}
return manufacturer;
}

@Override
public Manufacturer get(Long id) {
Copy link
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pay attention, according to the task, method should return Optional<Manufacturer>

Manufacturer manufacturer = new Manufacturer();
Copy link
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You need to create a Manufacturer instance after executing the query and verifying that the ResultSet is not empty

String getManufacturerRequest = "SELECT * FROM manufacturers WHERE id = ?";
try (Connection connection = ConnectionUtil.getConnection();
PreparedStatement getStatement =
connection.prepareStatement(getManufacturerRequest)) {
getStatement.setLong(1, id);
ResultSet resultSet = getStatement.executeQuery();
if (resultSet.next()) {
Long idOfManufacturer = resultSet.getLong("id");
String name = resultSet.getString("name");
String country = resultSet.getString("country");
manufacturer = new Manufacturer(idOfManufacturer, name, country);
Comment on lines +47 to +50
Copy link
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Try to avoid code duplication. Move retrieving data from ResultSet into Entity object to a separate private method and reuse it in other methods

}

} catch (SQLException e) {
throw new RuntimeException("Error");
}
return manufacturer;
}

@Override
public List<Manufacturer> getAll() throws RuntimeException {
Copy link
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You don't need to have throws RuntimeException in method signature

List<Manufacturer> manufacturerList = new ArrayList<>();
try (Connection connection = ConnectionUtil.getConnection();
Statement getAllStatement = connection.createStatement()) {
Copy link
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Use PreparedStatement over Statement, even for a static query with no parameters. It’s the best practice, and it’s slightly faster

ResultSet resultSet = getAllStatement
.executeQuery(
"SELECT manufacturers.id, manufacturers.name, manufacturers.country"
+ " FROM manufacturers WHERE is_deleted = 0");
Comment on lines +66 to +67
Copy link
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

  1. Use column names without table name
  2. Let’s use TRUE/FALSE instead of 1/0 with is_deleted column in the sql queries. The same in method delete()

while (resultSet.next()) {
String name = resultSet.getString("name");
String country = resultSet.getString("country");
Long id = resultSet.getObject("id", Long.class);
Manufacturer manufacturer = new Manufacturer(id, name, country);
manufacturerList.add(manufacturer);
Comment on lines +72 to +73
Copy link
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Don't create a redundant variables

}
} catch (SQLException e) {
throw new RuntimeException("Error");
}
return manufacturerList;
}

@Override
public void delete(Long id) {
Copy link
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

According to the task, method should return boolean
Don’t return true all the time. Return boolean value depending on executeUpdate() :
deletedRows > 0

String deleteRequest = "UPDATE manufacturers SET is_deleted = 1 where id = ?";
try (Connection connection = ConnectionUtil.getConnection();
PreparedStatement deleteStatement =
connection.prepareStatement(
deleteRequest, Statement.RETURN_GENERATED_KEYS)) {
Copy link
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Statement.RETURN_GENERATED_KEYS not needed in other methods except create()
The same mistake in method update()

deleteStatement.setLong(1, id);
deleteStatement.executeUpdate();
} catch (SQLException e) {
throw new RuntimeException("Error");
}
}

@Override
public Manufacturer update(Manufacturer manufacturer) {
String updateRequest = "UPDATE manufacturers SET name = ?, country = ? WHERE id = ?";
try (Connection connection = ConnectionUtil.getConnection();
PreparedStatement updateStatement =
connection.prepareStatement(
updateRequest, Statement.RETURN_GENERATED_KEYS)) {
updateStatement.setString(1, manufacturer.getName());
updateStatement.setString(2, manufacturer.getCountry());
updateStatement.setLong(3, manufacturer.getId());
updateStatement.executeUpdate();
return new Manufacturer();
Comment on lines +105 to +106
Copy link
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You need to check if update was successful: updatedRows > 0 and return the passed manufacturer (not a new one) if so or throw an DataProcessingException exception otherwise

} catch (SQLException e) {
throw new RuntimeException("Error");
}
}
}
45 changes: 45 additions & 0 deletions src/main/java/mate/jdbc/model/Manufacturer.java
Original file line number Diff line number Diff line change
@@ -0,0 +1,45 @@
package mate.jdbc.model;

public class Manufacturer {
private Long id;
private String name;
private String country;

public Manufacturer() {
}

public Manufacturer(String name, String country) {
this.name = name;
this.country = country;
}

public Manufacturer(Long id, String name, String country) {
this.id = id;
this.name = name;
this.country = country;
}

public Long getId() {
return id;
}

public void setId(Long id) {
this.id = id;
}

public String getName() {
return name;
}

public void setName(String name) {
this.name = name;
}

public String getCountry() {
return country;
}

public void setCountry(String country) {
this.country = country;
}
}
27 changes: 27 additions & 0 deletions src/main/java/mate/jdbc/util/ConnectionUtil.java
Original file line number Diff line number Diff line change
@@ -0,0 +1,27 @@
package mate.jdbc.util;

import java.sql.Connection;
import java.sql.DriverManager;
import java.sql.SQLException;
import java.util.Properties;

public class ConnectionUtil {
static {
try {
Class.forName("com.mysql.cj.jdbc.Driver");
Copy link
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

  1. Move this value to the constants
  2. I guess you forget to add mysql driver dependency in pom.xml

} catch (ClassNotFoundException e) {
throw new RuntimeException(e);
Copy link
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Use informative messages for exceptions Even like this.
For example: "Can't initialize MySQL driver"

}
}

public static Connection getConnection() {
try {
Properties dbProperties = new Properties();
dbProperties.put("user", "root");
dbProperties.put("password", "1234567");
return DriverManager.getConnection("jdbc:mysql://localhost:3306/taxi_db", dbProperties);
Comment on lines +20 to +22
Copy link
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Move this property names and values to the constants

} catch (SQLException e) {
throw new RuntimeException("Can't create connection to db", e);
}
}
}
9 changes: 9 additions & 0 deletions src/main/resources/init_db.sql
Original file line number Diff line number Diff line change
@@ -0,0 +1,9 @@
CREATE DATABASE `taxi_db`;
Copy link
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Using a ' ' for database/table/column name is optional


CREATE TABLE `manufacturers` (
`id` bigint NOT NULL AUTO_INCREMENT,
`name` varchar(255) DEFAULT NULL,
`country` varchar(255) DEFAULT NULL,
Comment on lines +4 to +6
Copy link
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

  1. Remember about SQL style: use uppercase for SQL keywords in your queries
  2. You don't have to specify NULL as a default value explicitly

`is_deleted` tinyint NOT NULL DEFAULT '0',
Comment on lines +4 to +7
Copy link
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let’s use TRUE/FALSE instead of 1/0 with is_deleted column

PRIMARY KEY (`id`)
Copy link
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's better to give an explicit name for constrains like: PRIMARY/FOREIGN KEYS and UNIQUE columns so that it is easier to identify the column causing the error if one occurs

) ENGINE=InnoDB DEFAULT CHARSET=utf8mb3