Return to Answer

• 1

• 1

• 6.7k
• 14
• 19

##General All of your classes should be final, as none of them are designed to be extended.

In java, static final member variables are named in uppercase. For instance, ExcelDB.datasource should be DATASOURCE.

##Datasource Don't use single-check locking in Datasource. You probably don't need lazy-load - just do private static final Datasource INSTANCE = new Datasource(). The only possible difference is if somebody statically references your class and doesn't call getInstance(), which is not currently possible. If you really need lazy load, use the holder idiom.

Datasource is questionable. If you're trying to hide the selected implementation of Repository from the client, use a Factory class which returns a Repository, and have ExcelDB, etc implement Repository.

Datasource should not expose instance as a public variable.

##Person Person has an identity crisis. Either it should be immutable, in which case there should be no setters and the member variables should be final, or it should be a bean, in which case there should be a no-arg constructor. Prefer the former when possible.

Person#toString() would be cleaner using String#format(), such as return String.format("Person{id=%d, name=%s}", this.id, this.name);

##ExcelDB ExcelDB should not be public, since the idea is to hide it behind the Repository interface. If it's public, classes outside the package can reference it directly, which is counter to your intent.

Why isn't datasource a File instead of a String?

Use try-with-resources to ensure closeables are handled correctly.

Always catch the most specific exception possible.

It's considered a good practice to hold a reference to a logger as either an instance or static member variable rather than fetching one as needed. It makes the code easier to read. You may also want to do yourself a favor and look into SLF4J/Logback, which are light-years ahead of java.util.logging.

row should be a method-level variable, not an instance variable.

find should be a boolean, not a Boolean, and it should be inside the method, not outside it.

I assume that POI needs the file open to access the workbook contents. If not, you should create a method that loads the workbook and then returns it, closing the file immediately rather than holding it through the loop.

The way to attack the repetition problem is to pass in a function that takes a XSSFRow and tells you whether or not you have a match. In java 8, you can do this directly. In java 7, you need to define an interface and concrete implementations. This gives you the flexibility to pass in different matchers besides just "column x = y".

If your data size is manageable, you might also consider just loading the whole file into memory in two Maps - a Map<Integer, Person> and a Map<String, Person>. This depends on the actual scope of work you need to do - much more feasible for a toy app or school assignment than a serious production environment with multiple massive files.

I threw together a few quick modifications to ExcelDB which might help kickstart your design:

final class ExcelDB implements Repository {
 private static final Logger LOGGER = Logger.getLogger(ExcelDB.class.getName());
 private static final File DATASOURCE = new File("src/doc/Classeur1.xlsx");
 public ExcelDB() {
 super();
 }
 @Override
 public Person getPersonById(final int id) {
 return this.queryExcel(new IntMatcher(0, id));
 }
 @Override
 public Person getPersonByName(final String name) {
 return this.queryExcel(new StringMatcher(1, name));
 }
 private Person queryExcel(final Matcher matcher) {
 try (final FileInputStream fis = new FileInputStream(DATASOURCE)) {
 final XSSFWorkbook workbook = new XSSFWorkbook(fis);
 final XSSFSheet spreadsheet = workbook.getSheetAt(0);
 final Iterator<Row> rowIterator = spreadsheet.iterator();
 // Skip header
 rowIterator.next();
 while (rowIterator.hasNext()) {
 final XSSFRow row = rowIterator.next();
 if (matcher.matches(row)) {
 return new Person(
 row.getCell(0).getNumericCellValue(),
 row.getCell(1).getStringCellValue());
 }
 }
 } catch (final IOException e) {
 LOGGER.log(Level.SEVERE, null, e);
 }
 return null;
 }
 private interface Matcher {
 boolean matches(final XSSFRow row);
 }
 private static final class IntMatcher implements Matcher {
 private final int column;
 private final int value;
 public IntMatcher(final int column, final int value) {
 this.column = column;
 this.value = value;
 }
 @Override
 public boolean matches(final XSSFRow row) {
 return row.getCell(this.column).getNumericCellValue() == this.value;
 }
 }
 private static final class StringMatcher implements Matcher {
 private final int column;
 private final String value;
 public StringMatcher(final int column, final String value) {
 this.column = column;
 this.value = value;
 }
 @Override
 public boolean matches(final XSSFRow row) {
 return row.getCell(this.column).getStringCellValue().equals(this.value);
 }
 }
}