General hints

[javagl]

As mentioned in the other issue: There are a million hints or suggestions for improvements that I could give. This may sound like a bold claim or even arrogant, but I'm willing to justify everything that I say here, on a technical level.

Some of the hints that I give here are quick ones, with short justifications. You might believe me, or just ignore them, or ask for further reasons. Also, the list may be a bit "unsorted". I'm creating it on the fly, while browsing over the code.

Add comments.

Really. Just write a short /** JavaDoc */ comment, at least for the classes and public methods. On the one hand, this is about documenting the state, to make it easier to maintain a healthy codebase in the long run. But it also helps to improve code quality. Imagine you have a method like moveBackForth and want to write a comment. When you have difficulties to write a comment (with 1 or 2 sentences) that succinctly explains what the method is doing, then this is a red flag. Maybe the method should be simplified or broken down into multiple methods.

Minimize visibility

Never use public or protected variables. Never use static variables. Never-ever use public static variables. When a method can be private or protected, then make it private or protected.

Non-private variables totally mess up encapsulation. You never know who is modifying the value of such a variable in which part of the code, at which point in time. (Note: This does refer to variables. Having a constant value (i.e. something that is final) is a different story. But even a public static final Point TRUE_COOR can be modified from the outside, so this still is a "variable" in some sense).

Minimize mutability and the state space

When a variable can be final, then make it final. This makes perfectly clear that the value may not change, and you don't have to think about whether it might change. Try to not expose a mutable state.

For example, consider the spriteList in the Map class:

public class Map extends JPanel {

    private ArrayList<Sprite> spriteList;

    public Map() {
       ...
    }
    
    public void initialize() {
        populateList();
        ...
    }


    public void populateList() {
        spriteList = new ArrayList<Sprite>();
        ...
    }
    
    public void moveMap(double deltax, double deltay) {
        ..
        for (Sprite p: spriteList) {
            p.moveWithMap(deltax, deltay);
        }
    }

    public ArrayList<Sprite> getSpriteList() {
        return spriteList;
    }
...
}

• Can spriteList be null? Sure it can.
• Who is calling initialize at which point in time?
• When somebody calls getSpriteList before initialize is called, then it may return null. This would have to be explained in a JavaDoc comment:
  /** @return The list of sprites, or null if initialize was not called, which happens... who knows when... */
• Is it OK to call map.getSpriteList().add(null)? Probably not. It will crash later with a NullPointerException.

Think about the Map class as being solely responsible for this spriteList. The Map class has to make sure that "everything works and nothing crashes". Some improvements:

public class Map extends JPanel {

    // Make it `final`
    private final List<Sprite> spriteList;

    public Map() {
       // Initialize it in the constructor
       this.spriteList = new ArrayList<Sprite>();
    }
    
    public void initialize() {
        populateList();
        ...
    }


    public void populateList() {
        // Don't create it here. Only populate it here.
        //spriteList = new ArrayList<Sprite>();
        ...
    }
    
    public void moveMap(double deltax, double deltay) {
        ..
        for (Sprite p: spriteList) {
            p.moveWithMap(deltax, deltay);
        }
    }

    // This is never null. It may not be modified by callers
    public List<Sprite> getSpriteList() {
        return Collections.unmodifiableList(spriteList);
    }
...
}

One point that is related to that: Try to avoid initialize methods. Who is calling these methods at which point in time? What happens when a caller randomly calls someObject.initialize() (even though it was already called before)? If possible, then every "initialization" should be done in the constructor.

Program against the interface

You may have seen that I changed the declaration from ArrayList<Sprite> to List<Sprite> in the snippet above. When you can use a more generic type, then you should do that. Interfaces are THE key for reusable, generic code. When you write a method like

public void doSomething(ArrayList<Sprite> list) { ... }

then it is not possible to write

LinkedList list = create();
doSomething(list); // Does not work. It expects an `ArrayList`

When writing the function as

public void doSomething(List<Sprite> list) { ... }

then you can use it for any list.

Never override the paint method of a Swing Component

This is done in Map#paint, for example. But in Swing, one should always override paintComponent instead. (The paint method does a few things that should not be omitted by overriding the method).

Avoid showing implementations in the public interface

This is a bit vague:

The MainPanel class currently looks like this:
public class MainPanel extends JPanel implements ActionListener, KeyListener { ... }

It can be beneficial and "cleaner" to move the ActionListener and KeyListener functionality into an own class. For example, you might have a class like

class PlayerControls implements KeyListener {
    private final MainCharacter mainCharacter;
    PlayerControls(MainCharacter mainCharacter) {
        this.mainCharacter = Objects.requireNonNull(
        	mainCharacter, "The mainCharacter may not be null");
    } 
    
    @Override
    public void keyPressed(KeyEvent e) {
    	// Like before
    }
    @Override
    public void keyReleased(KeyEvent e) {
    	// Like before
    }
    public void keyTyped(KeyEvent e) {
    	// Not used
    }
}

And in the MainPanel constructor, you could then do

        // Remove this 
        // addKeyListener(this); 

        // Add this after the mainCharacter is created:
        addKeyListener(new PlayerControls(mainCharacter)); 

This way, you might easily maintain the player controls in a single class (and maybe even a Multiplayer feature 😁 )

Avoid "shadowing" common names from the standard API.

You have classes that are called Point and Map. While it may be tempting to give them these simple names, it is really confusing when reading the code, and requires you to ´write java.awt.Point and java.util.Map when referring to the standard classes. Naming is hard. But instead of Map, you could consider something like GameMap or so. The Point class... is a different point, covered by the next point...

The Point class is basically a java.awt.geom.Point2D.Double

I know that the Point2D class can be a bit clumsy to use. And I know that you have added methods like move, translate and getMagnitude in the Point class. But using the standard Point2D class can simplify things a lot, and allows you to write code that is far more generic and applicable in many applications that already use the Point2D class. (I created some of these utility methods in my Geom library )

(The idea behind these methods is similar to your current normalize and addPoints methods - although I don't know why these methods are in the Constants class...)

Don't override equals inappropriately!!!

The Point#equals method is overridden to return false. This does not make sense. The equals method is a crucial method in the Object class, which has some strict constraints (also related to hashCode). You should not override this method in a way that violates this contract.

For example, consider the following code:

    	List<Point> points = new ArrayList<Point>();
    	Point point = new Point(12,34);
    	points.add(point);
    	System.out.println(points.contains(point));

The list obviously contains the given point. But still, the last line will print false. This is not good, and may cause really strange bugs.

Be careful about relations

This is a bit vague and abstract:

Right now, the MainCharacter class receives the MainPanel in its constructor and stores it. The MainPanel is not used in the MainCharacter class. Why is it there?

Even if there was a reason, like "I want to call this-and-that method of this panel", then you should wonder: Does the main character really need the panel? The panel probably needs the character. But the panel is "conceptually higher". If you had a class like House and a class like Door, then the House might contain a Door instance. But the Door does not have a House instance.

(There may be exceptions to this. Sometimes having something like an owner field can be fine. But one should keep the "direction of dependencies" in mind: Who owns what?)

Some of the geometric computations look clumsy...

This is also a bit vague. But I have seen many computations involving the m_turnAngle and lots of Math.PI and Math.cos/sin and some "quadrants" (that are enumerated using roman literals (I, II, III, IV) for whatever reason). It's hard to follow these computations. This difficulty may to some extent be caused by the fact that things like (m_turnAngle % (Math.PI/2) are repeated many times (12 times in the current codebase, to be exact).

I think that Sprite#move and MainCharacter#moveBackForth are essentially doing the same thing, by computing the deltax/y and the velocity.x/y, respectively. I just added the following to the MainCharacter#moveBackForth method:

    private static Point computeMovement(double angleRad, double speed)
    {
    	double x = Math.cos(angleRad);
    	double y = Math.sin(angleRad);
    	return new Point(x * speed, y * speed);
    }

    public void moveBackForth() {
        Point velocity = new Point();
        velocity.x = m_speed * Math.cos(m_turnAngle % (Math.PI/2));
        velocity.y = m_speed * Math.sin(m_turnAngle % (Math.PI/2));
        if (m_turnAngle >= (Math.PI/2) && m_turnAngle < Math.PI) { // quadrant II
            velocity.x = m_speed * Math.cos((Math.PI /2) - (m_turnAngle % (Math.PI/2)));
            velocity.y = m_speed * Math.sin((Math.PI/2) - (m_turnAngle % (Math.PI/2)));
            velocity.x = -velocity.x;
        } else if (m_turnAngle >= (Math.PI) && m_turnAngle < (3 * Math.PI / 2)) { // quadrant III
            velocity.x = -velocity.x;
            velocity.y = -velocity.y;
        } else if (m_turnAngle >= (3 * Math.PI / 2) && m_turnAngle < (2 *Math.PI)) { // quadrant IV
            velocity.x = m_speed * Math.cos((Math.PI/2)- (m_turnAngle % (Math.PI/2)));
            velocity.y = m_speed * Math.sin((Math.PI/2)- (m_turnAngle % (Math.PI/2)));
            velocity.y = -velocity.y;
        }
        
        Point velocitySimple = computeMovement(m_turnAngle, m_speed);
        System.out.println("velocity      : " + velocity);
        System.out.println("velocitySimple: " + velocitySimple);
...

And apparently, the velocity that is computed there is the same as what is computed as velocitySimple. (You may also call this computeMovement function in the Sprite class, to compute the deltax/y in a simpler way).

(An aside: Note that I called the parameter angleRad. Using such a suffix (as in angleRad or angleDeg) can make clear whether an angle is assumed to be in radians or degrees)

Consider inheritance instead of a 'type'

There currently is the HitBox class. It has a m_type value, and a createHitbox method. The code path in the createHitbox method only depends on the m_type. (And it does not cover the Constants.CIRCLE type yet). You could consider making the HitBox class abstract, and offer different implementations, ROUGHLY like that:

abstract class HitBox {

    abstract Point computeCollisionPoint();
}
class CowHitBox extends HitBox {
    @Override 
    public Point computeCollisionPoint() { 
        // What was done for m_type == Constants.COW, and call to SAT
    }
}
class RectangleHitBox extends HitBox {
    @Override 
    public Point computeCollisionPoint() { 
        // What was done for m_type == Constants.RECTANGLE, and call to SAT
    }
}

but the exact design here might require more thought.

---

I know, this was a lot. And I could just have opened a PR with a bunch of changes. But maybe it makes more sense to write this down here.

[FPErfvv]

This is a huge help, so thank you so much! I'll work on implementing these changes and if I have any questions I'll make sure to ask you.