-
Notifications
You must be signed in to change notification settings - Fork 12
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.
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
Algorithm 1 #3
base: master
Are you sure you want to change the base?
Algorithm 1 #3
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Very well written and structured code. But you could improve it by properly naming your variables (for instance using row and column instead of x and y), properly formating your comment (using javadoc style for method commenting). Finally, you didn't implement numberOfOpenSites
for (int i = 0; i < SIZE*SIZE; i++) { | ||
siteStatus[i] = false; | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is not needed as the default value of boolean is false.
|
||
// create N-by-N grid, with all sites blocked | ||
public Percolation(int N) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please could you check that the input N is valid?
return false; | ||
} | ||
return true; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You could improve this by returning directly a boolean expression.
return r >=1 && ...
private void connectTwoSite(int x1, int y1, int x2, int y2) { | ||
if (validIndex(x2, y2)) { | ||
if (isOpen(x2, y2)) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You could avoid enclosed if by using ANDed expression...
private int OIL_BELOW; | ||
|
||
// create N-by-N grid, with all sites blocked |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please could you use a javadoc like comment here
/** One line comment. */
|
||
// percolation experiment's grid width and height | ||
private int SIZE; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This variable is not static final (constant), then shouldn't be written in capital letters... Same for the two bellow...
if (!validIndex(i, j)) { | ||
throw new IndexOutOfBoundsException("row or/and column index out of bounds"); | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You're repeating this pattern three times, you could extract it to a checkIndex method and just call that one...
} | ||
|
||
// convert 2 dimensional(row, column) pair to 1 dimensional index |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please could you also use one line javadoc comment?
No description provided.