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.

Sign up for GitHub

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

refactor class group5 #18

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

refactor class group5 #18

Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
203 changes: 123 additions & 80 deletions src/main/java/org/springframework/samples/petclinic/rest/importcsv/ImportCSV.java
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,5 +1,6 @@
package org.springframework.samples.petclinic.rest.importcsv;

import jdk.javadoc.internal.doclets.toolkit.taglets.UserTaglet;
import org.springframework.beans.factory.annotation.Autowired;
import org.springframework.http.HttpHeaders;
import org.springframework.http.HttpStatus;
Expand All @@ -26,114 +27,156 @@ public class ImportCSV {
@Autowired
private ClinicService clinicService;

private int currentPosition;
Copy link

Choose a reason for hiding this comment

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

I guess you cannot know this but with this class variable you add 'state' to the class...before that, this controller and the importPets method were stateless


@PreAuthorize("hasRole(@roles.OWNER_ADMIN)")
@RequestMapping(value = "importPets",
method = RequestMethod.POST,
consumes = "text/plain",
produces = "application/json")
public ResponseEntity<List<Pet>> importPets(@RequestBody String csv) {

int i = 0;
this.currentPosition = 0;

List<Pet> pets = new LinkedList<Pet>();
Pet pet;
boolean petInformationAvailable;
Copy link

Choose a reason for hiding this comment

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

this method is maybe still a bit long (maybe because of time reasons)..think about extracting the different steps to a new method, so importPets only handles the http request, there is another method that handles the orchestration of parsing and saving data, and then there are multiple methods for parsing stuff and so on


do {
pet = new Pet();

String field = "";
while (i < csv.length() && csv.charAt(i) != ';') {
field += csv.charAt(i++);
}
i++;
setPetName(pet, csv);

pet.setName(field);
ResponseEntity<List<Pet>> invalidDate = setBirthDate(pet, csv);
if (invalidDate != null) return invalidDate;

field = "";
while (i < csv.length() && csv.charAt(i) != ';') {
field += csv.charAt(i++);
}
i++;

try {
pet.setBirthDate((new SimpleDateFormat("yyyy-MM-dd")).parse(field));
} catch (ParseException e) {
HttpHeaders headers = new HttpHeaders();
headers.add("errors", "date " + field + " not valid");
return new ResponseEntity<List<Pet>>(headers, HttpStatus.BAD_REQUEST);
}
setPetType(pet, csv);

field = "";
while (i < csv.length() && csv.charAt(i) != ';') {
field += csv.charAt(i++);
}
i++;

if (pet != null) {
ArrayList<PetType> ts = (ArrayList<PetType>) clinicService.findPetTypes();
for (int j = 0; j < ts.size(); j++) {
if (ts.get(j).getName().toLowerCase().equals(field)) {
pet.setType(ts.get(j));
break;
}
}
}
ResponseEntity<List<Pet>> invalidOwner = setPetOwner(pet, csv);
if (invalidOwner != null) return invalidOwner;

field = "";
while (i < csv.length() && (csv.charAt(i) != ';' && csv.charAt(i) != '\n')) {
field += csv.charAt(i++);
}
applyActionToPet(pet, csv);

if (pet != null) {
String owner = field;
List<Owner> matchingOwners = clinicService.findAllOwners()
.stream()
.filter(o -> o.getLastName().equals(owner))
.collect(Collectors.toList());

if (matchingOwners.size() == 0) {
HttpHeaders headers = new HttpHeaders();
headers.add("errors", "Owner not found");
return new ResponseEntity<List<Pet>>(headers, HttpStatus.BAD_REQUEST);
}
if (matchingOwners.size() > 1) {
HttpHeaders headers = new HttpHeaders();
headers.add("errors", "Owner not unique");
return new ResponseEntity<List<Pet>>(headers, HttpStatus.BAD_REQUEST);
}
pet.setOwner(matchingOwners.iterator().next());
}
this.currentPosition++;

if (csv.charAt(i) == ';') {
i++;
pets.add(pet);

field = "";
while (i < csv.length() && csv.charAt(i) != '\n') {
field += csv.charAt(i++);
}
petInformationAvailable = this.currentPosition < csv.length() && pet != null;
} while (petInformationAvailable);

return new ResponseEntity<List<Pet>>(pets, HttpStatus.OK);
}

if (field.toLowerCase().equals("add")) {
clinicService.savePet(pet);
} else {
for (Pet q : pet.getOwner().getPets()) {
if (q.getName().equals(pet.getName())) {
if (q.getType().getId().equals(pet.getType().getId())) {
if (pet.getBirthDate().equals(q.getBirthDate())) {
clinicService.deletePet(q);
}
}
}
private void removePetFromClinic(Pet pet) {
for (Pet q : pet.getOwner().getPets()) {
if (q.getName().equals(pet.getName())) {
if (q.getType().getId().equals(pet.getType().getId())) {
if (pet.getBirthDate().equals(q.getBirthDate())) {
clinicService.deletePet(q);
}
}
}
}
}

} else {
private void applyActionToPet(Pet pet, String csv) {
if (csv.charAt(this.currentPosition) == ';') {
this.currentPosition++;

String method = readNextField(csv);

if (method.toLowerCase().equals("add")) {
clinicService.savePet(pet);
} else {
removePetFromClinic(pet);
}
i++;

pets.add(pet);
} else {
clinicService.savePet(pet);
}
}

} while (i < csv.length() && pet != null);
private String readNextField(String csvString) {
Copy link

Choose a reason for hiding this comment

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

nice to have a different solution how to parse this (not splitting the string) :)

String field = "";
while (this.currentPosition < csvString.length() && (csvString.charAt(this.currentPosition) != ';' && csvString.charAt(this.currentPosition) != '\n')) {
field += csvString.charAt(this.currentPosition++);
}
return field;
}

return new ResponseEntity<List<Pet>>(pets, HttpStatus.OK);
private ResponseEntity<List<Pet>> generateErrorResponse(String errorMessage) {
HttpHeaders headers = new HttpHeaders();
headers.add("errors", errorMessage);
return new ResponseEntity<List<Pet>>(headers, HttpStatus.BAD_REQUEST);
}

private ResponseEntity<List<Pet>> setPetOwner(Pet pet, String csv) {
String petOwner = readNextField(csv);

if (pet != null) {
String owner = petOwner;
List<Owner> matchingOwners = getOwnerByLastName(owner);

ResponseEntity<List<Pet>> ownerNotFound = checkOwnerInDatabase(matchingOwners);
if (ownerNotFound != null) return ownerNotFound;

ResponseEntity<List<Pet>> ownerNotUnique = checkOwnerIsUnique(matchingOwners);
if (ownerNotUnique != null) return ownerNotUnique;

pet.setOwner(matchingOwners.iterator().next());
}
return null;
}

private List<Owner> getOwnerByLastName(String lastName) {
List<Owner> matchingOwners = clinicService.findAllOwners()
.stream()
.filter(o -> o.getLastName().equals(lastName))
.collect(Collectors.toList());
return matchingOwners;
}

private ResponseEntity<List<Pet>> checkOwnerInDatabase(List<Owner> owners) {
if (owners.size() == 0) {
return generateErrorResponse("Owner not found");
}
return null;
}

private ResponseEntity<List<Pet>> checkOwnerIsUnique(List<Owner> owners) {
if (owners.size() > 1) {
return generateErrorResponse("Owner not unique");
}
return null;
}

private void setPetName(Pet pet, String csv) {
String petName = readNextField(csv);
pet.setName(petName);
}

private ResponseEntity<List<Pet>> setBirthDate(Pet pet, String csv) {
String birthDate = readNextField(csv);
try {
pet.setBirthDate((new SimpleDateFormat("yyyy-MM-dd")).parse(birthDate));
} catch (ParseException e) {
return generateErrorResponse("date " + birthDate + " not valid");
}
return null;
}

private void setPetType(Pet pet, String csv){
Copy link

Choose a reason for hiding this comment

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

I like that you kept the indentations small throughout this class, maybe except in this method and in removePetFromClinic. If the indentations get too far, try to extract the loop- or if-bodies :)

String petType = readNextField(csv);

if (pet != null) {
ArrayList<PetType> ts = (ArrayList<PetType>) clinicService.findPetTypes();
for (int j = 0; j < ts.size(); j++) {
if (ts.get(j).getName().toLowerCase().equals(petType)) {
pet.setType(ts.get(j));
break;
}
}
}
}

}