patternjavaMinor
Project Euler #18 - Max path in a triangle
Viewed 0 times
pathtriangleprojectmaxeuler
Problem
I've been learning Java for the past 6 months online and haven't had anyone review my code before. I'm worried that I may be making lots of beginner mistakes and was wondering if anyone could point out any ideas on how to make the code more OO.
The class is instantiated from the main and all relevant code is here except for validation methods which are in super. My use of the constructor concerns me, but I don't see an obvious alternative, because my main will become a beast if I put the procedural stuff for 30 classes in there. Overall, the code feels pretty procedural, and a bit spaghetti. (moreso in this example than many, the interrelated triangles don't lend themselves to being black box)
Anyways, ideas, suggestions?
```
/*
* To change this license header, choose License Headers in Project Properties.
* To change this template file, choose Tools | Templates
* and open the template in the editor.
*/
package projecteuler;
/**
*
* @author Rar Rarr
*/
public class Problem18 extends Problems implements TalkNice{
private boolean useDefaultTriangle = false;
private int triangleSize;
private int [] triangle [];
private String [] pathingTriangle [];
private int[] printingTriangle[];
private String pathingInformation = new String();
Problem18(){
explainProblem();
useDefaultTriangle();
if(!useDefaultTriangle){
inputTriangleSize();
createPathingTriangle();
calculatePath(createTriangle());
}
else{
createPathingTriangle();
calculatePath(createDefaultTriangle());
}
printTriangle(triangle);
}
@Override
public final void explainProblem() {
System.out.println("By starting at the top of the triangle below and moving to adjacent numbers on the row below, the maximum total from top to bottom is 23.");
System.out.println("");
System.out.println("");
System.out.println("
The class is instantiated from the main and all relevant code is here except for validation methods which are in super. My use of the constructor concerns me, but I don't see an obvious alternative, because my main will become a beast if I put the procedural stuff for 30 classes in there. Overall, the code feels pretty procedural, and a bit spaghetti. (moreso in this example than many, the interrelated triangles don't lend themselves to being black box)
Anyways, ideas, suggestions?
```
/*
* To change this license header, choose License Headers in Project Properties.
* To change this template file, choose Tools | Templates
* and open the template in the editor.
*/
package projecteuler;
/**
*
* @author Rar Rarr
*/
public class Problem18 extends Problems implements TalkNice{
private boolean useDefaultTriangle = false;
private int triangleSize;
private int [] triangle [];
private String [] pathingTriangle [];
private int[] printingTriangle[];
private String pathingInformation = new String();
Problem18(){
explainProblem();
useDefaultTriangle();
if(!useDefaultTriangle){
inputTriangleSize();
createPathingTriangle();
calculatePath(createTriangle());
}
else{
createPathingTriangle();
calculatePath(createDefaultTriangle());
}
printTriangle(triangle);
}
@Override
public final void explainProblem() {
System.out.println("By starting at the top of the triangle below and moving to adjacent numbers on the row below, the maximum total from top to bottom is 23.");
System.out.println("");
System.out.println("");
System.out.println("
Solution
Just some points about the general structure, and a couple of smaller things that I noticed:
OOP
Constructor and general OOP
My use of the constructor concerns me, but I don't see an obvious alternative
Yes, it does seem a little odd. The constructor should only construct an object. Yours already solves the problem.
I would at least pull out
Does
You might also create an
It doesn't really make your main too big. It would look like this:
It has the advantage that you can easily only explain the problem or only solve the problem. And if you are worrying about a too big main method, extract that code into a function, so every problem becomes a one-liner.
Triangle
You might also consider creating a
Other
Your
Also, if you were using the switch, start with
Using
Don't use the
write
unnecessary temporary variables
You don't need to save values to a variable if you return them directly afterwards. Instead of
write
Spaces when using an Array
This:
Looks odd to me. The standard would be to not use spaces:
Magic Numbers
Don't hardcode special numbers. Instead, define them in a field with a proper name. For example, what is the significance of
Define variables where they are used
For example, you are defining
Method, Class, and Field comments
While some methods such as
Fields should also have a comment. For example, what is the difference between
Other comments
Inside methods, I would put more comments (but I sometimes like to write too many comments, so take it with a grain of salt). For example, why does
Also, the comments you do have start very far to the right. I would place them above the code they are commenting (otherwise, some people might have to scroll to read them).
OOP
Constructor and general OOP
My use of the constructor concerns me, but I don't see an obvious alternative
Yes, it does seem a little odd. The constructor should only construct an object. Yours already solves the problem.
I would at least pull out
explainProblem and the problem solving code (createPathingTriangle, calculatePath, printTriangle) to a method called solveProblem defined in Problems. Printing the solution could also be extracted, as it is a different activity from solving the problem.Does
verifyPositiveInt get user input? If so, that should also be pulled out of the constructor. Ideally, you would create a new interface that retrieves user input, instead of having Problems handle it.You might also create an
Output interface, so your code doesn't hardcode System.out.println. That way, you could easily switch to eg writing to a file.It doesn't really make your main too big. It would look like this:
Problem18 problem18 = new Problem18();
problem18.explainProblem();
problem18.applyUserInput(new ConsoleInput());
problem18.solveProblem();
problem18.printSolution(new ConsoleOutput());It has the advantage that you can easily only explain the problem or only solve the problem. And if you are worrying about a too big main method, extract that code into a function, so every problem becomes a one-liner.
Triangle
You might also consider creating a
Triangle class. Right now, the responsibility for maintaining a printable triangle is mixed in with the general code. Your code might be easier to read if this functionality was extracted. Other
createDefaultTriangleYour
for - switch seems entirely unneeded here. Just assign the values directly:triangle[0] = new int[]{4, 62, 98, 27, 23, 9, 70, 98, 73, 93, 38, 53, 60, 4, 23};Also, if you were using the switch, start with
y = 0, that way you don't need to subtract 1 from y everywhere.Using
String constructorDon't use the
String constructor. Instead of String str = new String("00" + String.valueOf(x));write
String str = "00" + String.valueOf(x);unnecessary temporary variables
You don't need to save values to a variable if you return them directly afterwards. Instead of
String str = "00" + String.valueOf(x);
return str;write
return "00" + String.valueOf(x);Spaces when using an Array
This:
triangle [y][x] += triangle [y - 1][x];Looks odd to me. The standard would be to not use spaces:
triangle[y][x] += triangle[y - 1][x];Magic Numbers
Don't hardcode special numbers. Instead, define them in a field with a proper name. For example, what is the significance of
4 - 2? The code doesn't tell me, but a properly named field might.Define variables where they are used
For example, you are defining
xAxis2 before a for loop, but it isn't used until after that loop. Move the definition to where it is actually used.Method, Class, and Field comments
While some methods such as
explainProblem don't necessarily need a comment, the entire class should have one (stating what the problem is), and most other methods should have one as well.Fields should also have a comment. For example, what is the difference between
triangle, pathingTriangle, printingTriangle, and pathingInformation? Ideally, anyone reading your code has at least a good idea what these variables will be used for without first understanding all your code.Other comments
Inside methods, I would put more comments (but I sometimes like to write too many comments, so take it with a grain of salt). For example, why does
y start at 1 instead of 0 in calculatePath? What does the y switch do in createDefaultTriangle, etc?Also, the comments you do have start very far to the right. I would place them above the code they are commenting (otherwise, some people might have to scroll to read them).
Code Snippets
Problem18 problem18 = new Problem18();
problem18.explainProblem();
problem18.applyUserInput(new ConsoleInput());
problem18.solveProblem();
problem18.printSolution(new ConsoleOutput());triangle[0] = new int[]{4, 62, 98, 27, 23, 9, 70, 98, 73, 93, 38, 53, 60, 4, 23};String str = new String("00" + String.valueOf(x));String str = "00" + String.valueOf(x);String str = "00" + String.valueOf(x);
return str;Context
StackExchange Code Review Q#61846, answer score: 4
Revisions (0)
No revisions yet.