Return to Revisions

Expose behavior / Don't manipulate the internal state directly

Object-orientation is about exposing behavior (not data), defining methods that are tailored for the problem at hand, that are meant to ensure data integrity and enable reuse of logic.

Take a look at this method, and tell what's wrong with it.

private void moveCursorToNextLocation() {
    final int saveX = currentCursor.x;
    final int saveY = currentCursor.y;
    
    while (true) {
        final int x = saveX + deltas[deltaIndex].x;
        final int y = saveY + deltas[deltaIndex].y;
    
        if (checkCurrentCursorIsValid(x, y)) {
            currentCursor.x = x;
            currentCursor.y = y;
            return;
        }
        getNextCursorDirection();
    }
}

Did you notice currentCursor.x, currentCursor.y and then currentCursor.x = x, currentCursor.y = y?

You're manually controlling the state of a dumb type IntPair instead of leveraging the Object-oriented paradigm through introducing useful behavior and properly encapsulating the data.

Another example in the very same method:

... + deltas[deltaIndex].x; // deltas[deltaIndex][0]
... + deltas[deltaIndex].y; // deltas[deltaIndex][1]

It looks almost as the code dialing with a nested array on the right, isn't? Objects seem to be in play, but we don't gain a lot.

Would it be nicer from the reader's perspective to see something like this:

... + xDelta();
... + yDelta();

And on the inside, xDelta() and yDelta() can perfectly well operate with an array of integers int[][] instead of IntPair[].

Refactoring

Your SnailMatrixGenerator class has a lot of knowledge about SnailMatrix, since it knows how to traverse it. Meanwhile, the noun Generator in its name suggests that it should be responsible for instantiation of SnailMatrix, basically matrix factory. But there's nothing intricate (at least for now) in obtaining the instance of SnailMatrix to warrant the existence of a factory.

Populating the matrix is a different concern, and the matrix factory should not dial with it.

In my opinion, abstraction that possesses knowledge on how to traverse and populate SnailMatrix should reside rather inside the SnailMatrix as a private nested class, than on the outside.

This way we can hide mutating method set() of SnailMatrix by making it private (it would be still visible to the nested class), and also hide this class tasked with initialization.

Based on the responsibility of this class, it makes sense to call it Initializer.

To populate the matrix, Initializer features the following methods:

• void set(int value) - initializes the element at the current position;
• boolean hasNext() - returns true if current position points to the uninitialized element;
• void next() - advances to the next position;

Here's how it might be implemented (deltas transformed into static field int[][] DIRECTIONS and deltaIndex became direction):

public class SnailMatrix {
    
    private final int[][] data;
    
    private SnailMatrix(int n) {
        this.data = new int[n][n];
    }
    
    public static SnailMatrix ofSize(int n) {
        var matrix = new SnailMatrix(n);
        var initializer = new MatrixInitializer(matrix);
        
        initialize(initializer);
        
        return matrix;
    }
    
    private static void initialize(MatrixInitializer initializer) {
        int value = 1;
        
        while (initializer.hasNext()) {
            initializer.set(value++);
            initializer.next();
        }
    }
    
    public int size() {
        return data.length;
    }
    
    public int get(final int x, final int y) {
        return data[y][x];
    }
    
    private void set(final int x, final int y, final int value) {
        data[y][x] = value;
    }
    
    // ...

    private static class MatrixInitializer {
        private static final int[][] DIRECTIONS =
            new int[][]{
                {1, 0}, {0, 1}, {-1, 0}, {0, -1}
            };
        private final SnailMatrix matrix;
        private int direction = 0;
        private int x;
        private int y;
        
        public MatrixInitializer(SnailMatrix matrix) {
            this.matrix = matrix;
        }
        
        public void set(int value) {
            matrix.set(x, y, value);
        }
        
        public boolean hasNext() {
            return  isValid(x, y)
                && !isInitialized();
        }
        
        private boolean isValid(int x, int y) {
            return x >= 0 && x < matrix.size()
                && y >= 0 && y < matrix.size();
        }
        
        private boolean isInitialized() {
            return isInitialized(x, y);
        }
        
        private boolean isInitialized(int x, int y) {
            return matrix.get(x, y) != 0;
        }
        
        private void next() {
            checkDirection();
            x += xDelta();
            y += yDelta();
        }
        
        private void checkDirection() {
            int nextX = x + xDelta();
            int nextY = y + yDelta();
            if (!isValid(nextX, nextY) || isInitialized(nextX, nextY)) {
                changeDirection();
            }
        }
        
        public int xDelta() {
            return DIRECTIONS[direction][0];
        }
        
        public int yDelta() {
            return DIRECTIONS[direction][1];
        }
        
        private void changeDirection() {
            direction = (direction + 1) % DIRECTIONS.length;
        }
    }
}

Avoid name parts that add no meaning

Avoid name parts that doesn't give new information about the code element.

Example:

private final IntPair currentCursor = new IntPair();

The expressiveness of the code would not suffer if we simply name it cursor.

Method name moveCursorToNextLocation can be shortened to moveCursor and still be informative. And when this method is being placed inside the class, solely task with traversal name next is sufficient to communicate its purpose.

Avoid magic numbers

private void getNextCursorDirection() {
    if (deltaIndex == 3) {
        // ...
}

3 is a "magic number", it should be deltas.length - 1 instead.