Skip to content
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

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

Algorithm 1 #3

wants to merge 1 commit into from

Conversation

EnochMbaebie
Copy link

No description provided.

Copy link

@erictoguem erictoguem left a 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;
}

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) {

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;

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)) {

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

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;

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");
}

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

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?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants