added solution to review #1
Conversation
| @@ -0,0 +1,9 @@ | |||
| CREATE DATABASE `taxi_db`; | |||
There was a problem hiding this comment.
Using a ' ' for database/table/column name is optional
| `id` bigint NOT NULL AUTO_INCREMENT, | ||
| `name` varchar(255) DEFAULT NULL, | ||
| `country` varchar(255) DEFAULT NULL, |
There was a problem hiding this comment.
- Remember about SQL style: use uppercase for SQL keywords in your queries
- You don't have to specify
NULLas a default value explicitly
| `id` bigint NOT NULL AUTO_INCREMENT, | ||
| `name` varchar(255) DEFAULT NULL, | ||
| `country` varchar(255) DEFAULT NULL, | ||
| `is_deleted` tinyint NOT NULL DEFAULT '0', |
There was a problem hiding this comment.
Let’s use TRUE/FALSE instead of 1/0 with is_deleted column
| `name` varchar(255) DEFAULT NULL, | ||
| `country` varchar(255) DEFAULT NULL, | ||
| `is_deleted` tinyint NOT NULL DEFAULT '0', | ||
| PRIMARY KEY (`id`) |
There was a problem hiding this comment.
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
| try { | ||
| Class.forName("com.mysql.cj.jdbc.Driver"); | ||
| } catch (ClassNotFoundException e) { | ||
| throw new RuntimeException(e); |
There was a problem hiding this comment.
Use informative messages for exceptions Even like this.
For example: "Can't initialize MySQL driver"
| } | ||
|
|
||
| @Override | ||
| public void delete(Long id) { |
There was a problem hiding this comment.
According to the task, method should return boolean
Don’t return true all the time. Return boolean value depending on executeUpdate() :
deletedRows > 0
| "SELECT manufacturers.id, manufacturers.name, manufacturers.country" | ||
| + " FROM manufacturers WHERE is_deleted = 0"); |
There was a problem hiding this comment.
- Use column names without table name
- Let’s use TRUE/FALSE instead of 1/0 with is_deleted column in the sql queries. The same in method
delete()
| updateStatement.executeUpdate(); | ||
| return new Manufacturer(); |
There was a problem hiding this comment.
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
| 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"); |
There was a problem hiding this comment.
- Use an informative variable names
- 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)); |
There was a problem hiding this comment.
- Override method
toString()in theManufacturerclass - Check if records were inserted by calling methods
get()andgetAll() - Check if record(s) were deleted by calling methods
delete()andget()
| Long idOfManufacturer = resultSet.getLong("id"); | ||
| String name = resultSet.getString("name"); | ||
| String country = resultSet.getString("country"); | ||
| manufacturer = new Manufacturer(idOfManufacturer, name, country); |
There was a problem hiding this comment.
Try to avoid code duplication. Move retrieving data from ResultSet into Entity object to a separate private method and reuse it in other methods
No description provided.