Skip to main content
Code Review

Return to Answer

Commonmark migration
Source Link

#Framework

Framework

#Naming

Naming

#Abstract class?

Abstract class?

#Generics

Generics

#Usefulness

Usefulness

#Framework

#Naming

#Abstract class?

#Generics

#Usefulness

Framework

Naming

Abstract class?

Generics

Usefulness

Source Link
Simon Forsberg
  • 59.7k
  • 9
  • 157
  • 311

#Framework

 /* **********************************
 * ADD YOUR PROBLEMS HERE!
 * ***********************************/

I'm sorry, did you just call this a framework? Never in my life have I seen a framework where I had to edit the sourcecode to make it usable.

Let users of the framework pass the List<Problem<?>> problems to the constructor instead.

And while you're at it, can I please have a public method from the ProjectEuler class? process for example, it'd be ideal as a public method.


#Naming

The ProjectEuler class isn't restricted to only problems that implements the EulerProblem interface, is it? I'm sure you can find a better name. ProblemBenchmarker perhaps?


#Abstract class?

I don't see the point of making Problem an abstract class. Personally, I think it makes more sense to add a field to the Problem class and a parameter to the constructor:

public Problem(String name, int warmups, int realruns, Supplier<T> supplier) {

And simply modify the execute method to:

public T execute() {
 return supplier.get();
}

This will provide the possibility to have multiple problems in the same class, which IMO provides a nice overview of the problems:

public static void main(String[] args) {
 List<Problem<?>> problems = new ArrayList<>();
 problems.add(new Problem<>("Averaging", 1000, 10000, ProblemMain::problemOne));
 problems.add(new Problem<>("Multiplying", 1000, 10000, ProblemMain::problemTwo));
 
 ProjectEuler euler = new ProjectEuler(problems);
 
 euler.process();
 System.out.println("\n\nWarmup Complete\n\n");
 euler.process();
}
private static final int[] DATA = {1, 2, 3, 4, 5, 6, 7, 8, 9, 8, 7, 6, 5, 4, 3, 2, 1};
public static Double problemOne() {
 int sum = 0;
 for (int v : DATA) {
 sum += v;
 }
 return sum / (double)DATA.length;
}
public static Integer problemTwo() {
 int sum = 0;
 for (int v : DATA) {
 sum *= v;
 }
 return sum;
}

#Generics

I'm not so sure that the generics of the Problem<T> class does you any good. It's used in a List<Problem<?>> anyway so I don't see that the type safety of generics gives you anything. Consider removing the generics and use a Supplier<?> inside it instead, and have the execute() method return an Object.

Of course it'd be neat if it was possible to avoid the auto-boxing that Java does on primitive values, but I expect that would require some code duplication, and I'm not sure if the potential performance gain you could get out of it is worth the code to add it. (I bet you can answer that better than I can, but you have taught me that auto-boxing does affect the performance a bit)


#Usefulness

Overall, I think this code will be extremely useful, especially when I can use it without having to modify it's source!

lang-java

AltStyle によって変換されたページ (->オリジナル) /