Skip to main content
Code Review

Return to Answer

added 119 characters in body
Source Link

Your "main" class is a component that extends JFrame. I know that code examples in books and tutorials are full of examples like that. But this is bad practice and not proper OO: It violates the LSP (Liskov Substitution Principle; Barbara Liskov justifiedly received a Turing award for coming up with this) because your main class cannot be reused the same way as a JFrame. In layman's terms, subclasses should always represent adequate substitutes for their superclasses. And it is not necessary at all to extend JFrame. You can just fine do something like JFrame frame = new JFrame() and then call its methods.

Your "main" class is a component that extends JFrame. I know that code examples in books and tutorials are full of examples like that. But this is bad practice and not proper OO: It violates the LSP (Liskov Substitution Principle; Barbara Liskov justifiedly received a Turing award for coming up with this) because your main class cannot be reused the same way as a JFrame. In layman's terms, subclasses should always represent adequate substitutes for their superclasses. And it is not necessary.

Your "main" class is a component that extends JFrame. I know that code examples in books and tutorials are full of examples like that. But this is bad practice and not proper OO: It violates the LSP (Liskov Substitution Principle; Barbara Liskov justifiedly received a Turing award for coming up with this) because your main class cannot be reused the same way as a JFrame. In layman's terms, subclasses should always represent adequate substitutes for their superclasses. And it is not necessary at all to extend JFrame. You can just fine do something like JFrame frame = new JFrame() and then call its methods.

Source Link

The first thing that I notice when looking at the code is something that directly stings in my eyes: Formatting. The code is is not formatted according to the SUN or Google Code Conventions (which are largely identical). The opening curly braces are misplaced, and the else is misplaced. Also, the indentation is inconsistent. Some blocks are indented 2, some 3, some 4 spaces. The visual appearance of Java code should be an indentation of 4 spaces.

The second thing which I noticed right away is that there are no tests.

The third thing is that your implementation of Game of Life is limited to a finite universe: the width and height of the universe must be known upfront. It is possible to create an implementation that supports an infinite universe. And if you like Game of Life (I do), you may find it a very interesting and enlightening challenge to attempt yourself at such an implementation.

Use descriptive names for variables. In your code, unless i really is just an anonymous counter, it's better to use x and y or row and col. And these names should be consistent throughout the code. I see sometimes i and j, sometimes p and m, sometimes row and column in your code.

Same goes for width and height, for which I sometimes see N and M instead.

The this qualifier should be omitted unless it is necessary to resolve ambiguity or otherwise communicate intent.

Your "main" class is a component that extends JFrame. I know that code examples in books and tutorials are full of examples like that. But this is bad practice and not proper OO: It violates the LSP (Liskov Substitution Principle; Barbara Liskov justifiedly received a Turing award for coming up with this) because your main class cannot be reused the same way as a JFrame. In layman's terms, subclasses should always represent adequate substitutes for their superclasses. And it is not necessary.

Besides, GameOfLife is a bad name for something that extends JFrame. It should be possible to make an educated guess about the class hierarchy from the class name. The class name GameOfLife has nothing in it that suggests that it is a JFrame, nor anything that suggests that this is the class with the main() method.

Same goes for Cells. The name Cells does not suggest to the reader that this class is a UI component.

The superclass for Cells should be JComponent, not JPanel. The purpose of a JPanel is that you can set its layout manager and add components. Using JPanel instead of JComponent is again an LSP violation.

You could use more and thus smaller methods. This would allow you to have less duplication, more code reuse, and thus also fewer errors. For example, look at the constructor Universe(). It includes a section of code that initializes the entire universe with random bits. The method reset() does the same. You could extract this into a method randomize(), like this:

 public void randomize(int height, int width, int seed) {
 for (int y = 0; y < h; y++) {
 for (int x = 0; x < h; x++) {
 currentGeneration[y][x] = random.nextBoolean();
 }
 }
 }

You could call this randomize() method from both, reset() and Universe().

You might want to prefer method references over anonymous lambdas if you do not need Java's half-assed closures (access to variables of the enclosing method; in Java half-assed because they must be effectively final). This makes your code cleaner.

Fields which are initialized with a field initializer or with an assignment in the constructor but never assigned again should be final. If you want to write really good code, then most of your data will be final.

calculateNeighbors() is always called with currentGrid as its first argument. Remove the argument and make calculateNeighbors() an instance method. Same for calculateAlive().

In calculateNeighbors(), the code

 if (r < 0)
 r = N - 1;
 if (r > N - 1)
 r = 0;
 if (c < 0)
 c = M - 1;
 if (c > M - 1)
 c = 0;

can be simplified significantly:

 r = (r + N) % N;
 c = (c + M) % M;

(x + r) % r is the general formula to ensure for x ∈ Z, r ∈ N that 0 <= x < r. Besides, this simplification will ensure the expected (torus universe) behavior in case you want to support a ruleset with a neighbor distance > 1.

In the methods generateNthGeneration(), X (uppercase) is used as a parameter name. This is misleading: A single uppercase letter is expected to be a type or a constant, but not a (in this case parameter) variable.

In your repaint, you have this code:

cells.grid = universe.getCurrentGeneration();

The class Cells should be able to render the correct generation without having another class (GameOfLife) helping with it. For that, class Cells should directly refer to class Universe, not its grid[][].

Overall, look out for duplication and remove it.

Also, look out for misplaced responsibility. You can detect misplaced responsibility by using ., especially when using it multiple times. There is a princple called Law of Demeter that can help you with identifying misplaced responsibility. You will notice that when you fix misplaced responsibility by moving things in the right places, that lines become shorter.

Remove unused code. Method getNextGeneration() is never used.

In method generateNextGeneration(), you may want to use a separate class to determine whether a cell survives or is born. This would allow you to easily implement other rulesets. Conway's Game of Life is B3/S23. Another popular ruleset is Highlife, B36/S23. The design pattern to do that is called Strategy.

lang-java

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