Spring MVC, Mongodb and java

Question

I am avery beginner at SPring MVC, Mongodb and java. I am trying to make a sample application so that the user can serach, create, update and delete a customer:

My CustomerDAO class:

package com.example;
import com.mongodb.BasicDBObject;
import com.mongodb.DBCursor;
import com.mongodb.MongoClient;
import com.mongodb.client.MongoCollection;
import com.mongodb.client.MongoCursor;
import com.mongodb.client.MongoDatabase;
import java.util.ArrayList;
import java.util.List;
import org.bson.Document;
import static org.glassfish.hk2.utilities.reflection.Pretty.collection;
import static org.springframework.core.convert.TypeDescriptor.collection;
import org.springframework.stereotype.Component;
@Component
public class CustomerDAO 
{
 private ArrayList<Customer> customers;
 public CustomerDAO()
 {
 customers = new ArrayList();
 }
 public ArrayList<Customer> getCustomers()
 {
 MongoClient mongoClient = new MongoClient("localhost", 27017);
 MongoDatabase database = mongoClient.getDatabase("testdb");
 MongoCollection<Document> col = database.getCollection("customer");
 MongoCursor<Document> cur = col.find().iterator();
 while(cur.hasNext())
 {
 Document doc = cur.next();
 List list = new ArrayList(doc.values());
 customers.add(new Customer((int) Float.parseFloat(list.get(1).toString()), list.get(2).toString(), list.get(3).toString())); 
 }
 mongoClient.close();
 return customers;
 }
 public ArrayList<Customer> get(int id)
 {
 MongoClient mongoClient = new MongoClient("localhost", 27017);
 MongoDatabase database = mongoClient.getDatabase("testdb");
 MongoCollection<Document> col = database.getCollection("customer"); 
 BasicDBObject queary = new BasicDBObject("id", new BasicDBObject("$eq", id));
 MongoCursor<Document> cur = col.find(queary).iterator();
 while(cur.hasNext())
 {
 Document doc = cur.next();
 List list = new ArrayList(doc.values());
 customers.add(new Customer((int) Float.parseFloat(list.get(1).toString()), list.get(2).toString(), list.get(3).toString())); 
 }
 mongoClient.close();
 return customers;
 }
 public ArrayList<Customer> delete(Long id)
 {
 MongoClient mongoClient = new MongoClient("localhost", 27017);
 MongoDatabase database = mongoClient.getDatabase("testdb");
 MongoCollection<Document> col = database.getCollection("customer"); 
 BasicDBObject queary = new BasicDBObject("id", new BasicDBObject("$eq", id));
 col.deleteOne(eq("id", id)); 
 mongoClient.close();
 return customers;
 }
 public ArrayList<Customer> update(Long id)
 {
 MongoClient mongoClient = new MongoClient("localhost", 27017);
 MongoDatabase database = mongoClient.getDatabase("testdb");
 MongoCollection<Document> col = database.getCollection("customer"); 
 BasicDBObject queary = new BasicDBObject("id", new BasicDBObject("$eq", id));
 MongoCursor<Document> cur = col.find(queary).iterator();
 while(cur.hasNext())
 {
 Document doc = cur.next();
 List list = new ArrayList(doc.values());
 customers.add(new Customer((int) Float.parseFloat(list.get(1).toString()), list.get(2).toString(), list.get(3).toString())); 
 }
 mongoClient.close();
 return customers;
 }
}

My CustomerRestController class:

package com.example;
import java.util.ArrayList;
import java.util.List;
import org.springframework.beans.factory.annotation.Autowired;
import org.springframework.http.HttpStatus;
import org.springframework.http.ResponseEntity;
import org.springframework.web.bind.annotation.CrossOrigin;
import org.springframework.web.bind.annotation.GetMapping;
import org.springframework.web.bind.annotation.PathVariable;
import org.springframework.web.bind.annotation.RequestBody;
import org.springframework.web.bind.annotation.RequestMapping;
import org.springframework.web.bind.annotation.RestController;
import static org.springframework.web.bind.annotation.RequestMethod.DELETE;
import static org.springframework.web.bind.annotation.RequestMethod.GET;
import static org.springframework.web.bind.annotation.RequestMethod.POST;
import static org.springframework.web.bind.annotation.RequestMethod.PUT;
@RestController
public class CustomerRestController 
{
 @Autowired
 private CustomerDAO customerDAO;
 @CrossOrigin
 @GetMapping("/customers")
 public ArrayList<Customer> getCustomers()
 {
 customerDAO = new CustomerDAO();
 return customerDAO.getCustomers();
 }
 @CrossOrigin
 @GetMapping("/customers/{id}")
 public ArrayList<Customer> getCustomer(@PathVariable("id") int id)
 {
 customerDAO = new CustomerDAO();
 return customerDAO.get(id);
 } 
 @CrossOrigin
 @PostMapping(value = "/customers")
 public ArrayList<Customer> createCustomer(@RequestBody Customer customer) {
 customerDAO = new CustomerDAO();
 return customerDAO.create(customer);
 }
 @CrossOrigin
 @DeleteMapping("/customers/{id}")
 public ArrayList<Customer> deleteCustomer(@PathVariable Long id) {
 customerDAO = new CustomerDAO();
 return customerDAO.delete(id);
 }
 @CrossOrigin
 @PutMapping("/customers/{id}")
 public ArrayList<Customer> updateCustomer(@PathVariable Long id, @RequestBody Customer customer) {
 customerDAO = new CustomerDAO();
 return customerDAO.update(id, customer);
 }
}

My restclient page:

<html>
<head>
 <script src="https://ajax.googleapis.com/ajax/libs/jquery/3.1.1/jquery.min.js"></script>
</head>
<body>
 <input name="search" type="text" maxlength="100" id="search"/>
 <button onclick="searchID()"> Search ID </button>
 <button onclick="saveID()"> Save ID </button>
 <button onclick="deleteID()"> Delete ID </button>
 <button onclick="updateID()"> Update ID </button>
 <button onclick="showAll()"> Show All </button>
 <div id="persons"></div>
 <script>
 function searchID()
 {
 var id = document.getElementById("search").value;
 $("#persons").html("");
 $.getJSON("http://localhost:8080/customers/" + id, function(data)
 {
 for (var i in data) {
 $('#persons').append("<p>ID: " + data[i].id + "</p>")
 $('#persons').append("<p>First name: " + data[i].firstName + "</p>")
 $('#persons').append("<p>Last name: " + data[i].lastName + "</p><br>")
 }
 });
 }
 function showAll()
 {
 $("#persons").html("");
 $.getJSON("http://localhost:8080/customers/", function(data)
 {
 for (var i in data) {
 $('#persons').append("<p>ID: " + data[i].id + "</p>")
 $('#persons').append("<p>First name: " + data[i].firstName + "</p>")
 $('#persons').append("<p>Last name: " + data[i].lastName + "</p><br>")
 }
 });
 }
 </script>
</body>
</html>

• Simon Forsberg: Welcome to Code Review. Just to clarify: Does this code work as expected? Are you looking for anything specific from us?

Answer 1

CustomerDAO:

1. You should create a single MongoClient that your DAO uses to access the database, as suggested in the documentation. The client internally manages connections to the underlying database so all the opening and closing you perform is unnecessary and inefficient. To go even further, you should have Spring manage it and inject it to whatever needs it.

2. queary is a typo, the correct spelling is query.

3. list is not a very descriptive name. We already know it's a List, the type is right there. Better something like documentValues that conveys meaning and even that's not a great one.

4. That said, transforming the documents into raw lists of values and extracting the values via index is rather opaque and fragile. The indices used look like magic numbers, without meaning attached, while a Documentis already a Map. Thus you could, for example, call getInteger('id') on it instead of juggling indices. This will lets the reader know that the value is meant to be an integer and that it is the id of the object.

5. I would assume by the name and parameter that the get method will return a Customer instance with that ID. Instead, it queries the database, the resulting instance is pulled out and added to some sort of local cache and the entire cache is returned. Note that this is identical to the getCustomers method; the only difference between these two methods is the hidden side effects they're doing behind the scenes.

6. customers currently doesn't actually do anything except provide lots of pitfalls to stumble into. It looks like some sort of cache yet every method bypasses it to always hit the database. Every method manipulates and returns it so every action has the danger of stepping on each others toes. It's also possible for classes outside the DAO to manipulate it because the mutable list is returned directly. You honestly should just get rid of it.

7. The update method doesn't update anything; it is exactly the same as the get method with a different name. So we now have 3 methods with different names and the same obvious effect but subtly different hidden side effects.

8. There is no method that suggests to me that it is meant to be used to save new values. I would replace the update method with a save that is used to both add new values or update existing ones to the database like Spring uses.

Controller:

1. You are using @Autowired to inject the DAO from the container, but in every handler method you are throwing out the instance you already have and creating a new one.

2. createCustomer refers to the create DAO method, which doesn't exist.

3. updateCustomer tries to call the update method on the DAO with the wrong signature.

restClient.html:

1. You're mixing jQuery and native JavaScript to get DOM elements, you should stick to just the jQuery.

2. Your scripts should be in separate files, not inline.