https://github.com/lesfuretshttps://github.com/mgandin@BeastieFurets@m_gandin
Our code review process is based on a git flow branching strategy and does not require any tool.
You request a code review by changing the status in JIRA to code review (lesfurets.com), by doing a pull request (github) or a merge request (gitlab).
(1) Style and naming
(2) Language and API usage
(3) Simplicity
(4) Object oriented programming
void check(MainDriver d1, Driver d2, Besoins b)
throws ExclusionException {
int age = DateHelper.getAge(driver.getBirthDate(),
DateHelper.getToday(),
EPrecision.day);
if (age < 18)
throw new ExclusionException("Must be at least 18 years old");
}
// Use precise method naming, parameter naming,
// and remove unused parameters
void check(MainDriver d1, Driver d2, Requirements r)
throws ExclusionException {
// Could use static imports if clearer and correct indentation
int age = DateHelper.getAge(driver.getBirthDate(),
DateHelper.getToday(),
EPrecision.day);
// Maybe use constants for magic numbers and string
if (age < 18)
// Use brackets for single line statements
throw new ExclusionException("Must be at least 18 years old");
}
void checkAge(MainDriver mainDriver) throws ExclusionException {
int age = getAge(driver.getBirthDate(), getToday(), EPrecision.day);
if (age < MINIMUM_AGE) {
throw new ExclusionException("Must be at least 18 years old");
}
}
private boolean isPreviousVersion(String version, String compareWith) {
String[] versionDigits = compare.split("\\.");
String[] compareDigits = compareWith.split("\\.");
if (valueOf(versionDigits[0]) < valueOf(compareDigits[0]))
return true;
else if (valueOf(versionDigits[0]) == valueOf(compareDigits[0])) {
if (valueOf(versionDigits[1]) < valueOf(compareDigits[1]))
return true;
else {
if (valueOf(versionDigits[1]) == valueOf(compareDigits[1]))
return valueOf(versionDigits[2]) < valueOf(compareDigits[2]);
return false;
}
}
return false;
}
// General remark: you only compare the first 3 numbers?
private boolean isPreviousVersion(String version, String compareWith) {
String[] versionDigits = compare.split("\\.");
String[] compareDigits = compareWith.split("\\.");
if (valueOf(versionDigits[0]) < valueOf(compareDigits[0]))
return true;
// Do not compare Integer with == (it only works up to 127)
// either use int instead, or use .equals if Integer
else if (valueOf(versionDigits[0]) == valueOf(compareDigits[0])) {
if (valueOf(versionDigits[1]) < valueOf(compareDigits[1]))
return true;
else {
// Exact same behaviour as above, use private method
if (valueOf(versionDigits[1]) == valueOf(compareDigits[1]))
return valueOf(versionDigits[2]) < valueOf(compareDigits[2]);
return false;
}
}
// A lot of return statement, isn't there a simpler way?
return false;
}
private boolean isPreviousVersion(String version, String compareWith) {
String[] versionDigits = compare.split("\\.");
String[] compareDigits = compareWith.split("\\.");
for (int i = 0; i < versionDigits.length; i++) {
if (isEquals(versionDigits[i], compareDigits[i])) {
continue;
}
return isPrevious(versionDigits[i], compareDigits[i]);
}
return false;
}
private boolean isEquals(String versionNumber, String compareWith) {
return Integer.parseInt(versionNumber) == Integer.parseInt(compareWith);
}
Collection<Object> removeDuplicates(Collection<?> collection) {
List<Object> uniques = new ArrayList<>();
for (Object object : collection)
if (!uniques.contains(object))
uniques.add((object));
return uniques;
}
Date addDaysToDate(Date date, int numberOfDays) {
Calendar calendar = Calendar.getInstance();
calendar.setTime(date);
calendar.add(Calendar.DAY_OF_YEAR, numberOfDays);
return calendar.getTime();
}
Collection<Object> removeDuplicates(Collection<?> collection) {
// You could use a set to remove duplicates
List<Object> uniques = new ArrayList<>();
for (Object object : collection)
if (!uniques.contains(object))
uniques.add(object);
return uniques;
}
Date addDaysToDate(Date date, int numberOfDays) {
// You could use the new Java 8 LocalDate API
Calendar calendar = Calendar.getInstance();
calendar.setTime(date);
calendar.add(Calendar.DAY_OF_YEAR, numberOfDays);
return calendar.getTime();
}
Collection<Object> removeDuplicates(Collection<?> collection) {
return new HashSet<>(collection);
}
LocalDate addDaysToDate(LocalDate date, int numberOfDays) {
return date.plus(numberOfDays, DAYS);
}
public BigDecimal recomputePrice(Quote quote) throws ComputationPriceException {
BigDecimal quoteTotalPrice = new BigDecimal(quote.getPrice().doubleValue());
if (quote.getWarrantyAvailability1().equals(quote.getQuoteType()))
quoteTotalPrice = quoteTotalPrice.add(quote.getWarrantyPrice1());
if (quote.getWarrantyAvailability2().equals(quote.getQuoteType()))
quoteTotalPrice = quoteTotalPrice.add(quote.getWarrantyPrice2());
if (quote.getExpirationDate().isBefore(LocalDate.now()))
throw new ComputationPriceException("It's too late !");
if (!quote.isFeesIncluded()) {
if (quote.getOneTimeFees() != null)
quoteTotalPrice = quoteTotalPrice.add(quote.getOneTimeFees());
if (quote.getAnnualFees() != null)
quoteTotalPrice = quoteTotalPrice.add(quote.getAnnualFees());
}
// Taxes has changed after 1st January 2015
if (quote.getDate().isBefore(LocalDate.of(2015, 1, 1)))
quoteTotalPrice = quoteTotalPrice.multiply(new BigDecimal(1.20));
else
quoteTotalPrice = quoteTotalPrice.multiply(new BigDecimal(1.196));
// Rounding
return quoteTotalPrice.setScale(2, BigDecimal.ROUND_HALF_UP);
}
public BigDecimal recomputePrice(Quote quote) throws ComputationPriceException {
// Variables with numbers is rarely a good option, use a list instead?
// Warranties look independant from the quote, extract in an object
BigDecimal quoteTotalPrice = new BigDecimal(quote.getPrice().doubleValue());
if (quote.getWarrantyAvailability1().equals(quote.getQuoteType()))
quoteTotalPrice = quoteTotalPrice.add(quote.getWarrantyPrice1());
if (quote.getWarrantyAvailability2().equals(quote.getQuoteType()))
quoteTotalPrice = quoteTotalPrice.add(quote.getWarrantyPrice2());
// Use fail fast that avoid executing the function if not necessary
if (quote.getExpirationDate().isBefore(LocalDate.now()))
throw new ComputationPriceException("It's too late !");
// Encapsulate implementation, we don't need to know about it here
if (!quote.isFeesIncluded()) {
if (quote.getOneTimeFees() != null)
quoteTotalPrice = quoteTotalPrice.add(quote.getOneTimeFees());
if (quote.getAnnualFees() != null)
quoteTotalPrice = quoteTotalPrice.add(quote.getAnnualFees());
}
// Same remark as above
// Taxes has changed after 1st January 2015
if (quote.getDate().isBefore(LocalDate.of(2015, 1, 1)))
quoteTotalPrice = quoteTotalPrice.multiply(new BigDecimal(1.20));
else
quoteTotalPrice = quoteTotalPrice.multiply(new BigDecimal(1.196));
// Rounding
return quoteTotalPrice.setScale(2, BigDecimal.ROUND_HALF_UP);
}
public BigDecimal recomputePrice(Quote quote) throws ComputationPriceException {
checkQuoteIsStillValid(quote.getExpirationDate());
BigDecimal quoteTotalPrice = new BigDecimal(quote.getPrice().doubleValue())
.add(warrantyService.computeWarrantiesAmount(quote.getWarranties()))
.add(feesService.computeFeesAmount(quote.getFees()));
return taxesService.applyTaxes(quote.getDate(), quoteTotalPrice);
}