Top-down movement

Question

I've been working on a Final Fantasy-ish game and am leaning toward more like 1 - 3. This is my attempt at map movement. It does work, but if anyone has any suggestions or critiques, I would like to hear them. For instance, is there a different way you would do something or is there a way I can clean up the code?

package com.stardust.main;
import java.awt.Graphics; 
import java.awt.event.KeyEvent; 
import java.awt.event.KeyListener; 
import java.awt.image.BufferedImage;
public class Player implements KeyListener 
{ 
 //Location
 private int x, y; 
 //Size Variables 
 private int sx, sy; 
 //Movement tests 
 public boolean up = false, dn = false, lt = false, rt = false; 
 public int SPEED = 3; 
 //More movement test mToPMove() stands for map to player movement up, down, left, right 
 private boolean mToPMoveU = false, mToPMoveD = false, mToPMoveL = false, mToPMoveR = false;
 public BufferedImage image;
 public Player(int x, int y, int sx, int sy, BufferedImage image)
 {
 this.x = x;
 this.y = y;
 this.sx = sx;
 this.sy = sy;
 this.image = image;
 }
 public void update()
 {
 //here map moves because player is in center of map
 if(up && !mToPMoveU)
 {
 Game.getnewBackground().y += SPEED; 
 }
 if(dn && !mToPMoveD)
 {
 Game.getnewBackground().y -= SPEED;
 }
 if(lt && !mToPMoveL)
 {
 Game.getnewBackground().x += SPEED;
 }
 if(rt & !mToPMoveR)
 {
 Game.getnewBackground().x -= SPEED;
 }
 //check when map hits edges of frame, then start player movement
 checkMapPosition();
 if(up && mToPMoveU)
 {
 Game.getPlayer().y -= SPEED;
 }
 if(dn && mToPMoveD)
 {
 Game.getPlayer().y += SPEED;
 }
 if(lt && mToPMoveL)
 {
 Game.getPlayer().x -= SPEED;
 }
 if(rt && mToPMoveR)
 {
 Game.getPlayer().x += SPEED;
 }
 //check when player hits edge of map
 checkPlayerPosition();
 //when player gets back to the center of the map, start map movement again
 reverseMove();
 }
 public void render(Graphics g)
 {
 g.drawImage(image, x, y, sx, sy, null);
 }
 public void keyTyped(KeyEvent e)
 {
 }
 public void keyPressed(KeyEvent e)
 {
 if(e.getKeyCode() == KeyEvent.VK_UP)
 {
 up = true; 
 }
 if(e.getKeyCode() == KeyEvent.VK_DOWN)
 {
 dn = true;
 }
 if(e.getKeyCode() == KeyEvent.VK_LEFT)
 {
 lt = true;
 }
 if(e.getKeyCode() == KeyEvent.VK_RIGHT)
 {
 rt = true;
 }
 }
 public void keyReleased(KeyEvent e)
 {
 if(e.getKeyCode() == KeyEvent.VK_UP)
 {
 up = false;
 }
 if(e.getKeyCode() == KeyEvent.VK_DOWN)
 {
 dn = false;
 }
 if(e.getKeyCode() == KeyEvent.VK_LEFT)
 {
 lt = false;
 }
 if(e.getKeyCode() == KeyEvent.VK_RIGHT)
 {
 rt = false;
 }
 }
 public void checkMapPosition()
 {
 if(Game.getnewBackground().y >= 0)
 {
 Game.getnewBackground().y = 0;
 mToPMoveU = true;
 mToPMoveD = true;
 }
 if(Game.getnewBackground().y <= Game.getFrameSize().getHeight() - (Game.getnewBackground().sy + 30))
 {
 Game.getnewBackground().y = Game.getFrameSize().getHeight() - (Game.getnewBackground().sy + 30);
 mToPMoveD = true;
 mToPMoveU = true;
 }
 if(Game.getnewBackground().x >= 0)
 {
 Game.getnewBackground().x = 0;
 mToPMoveL = true;
 mToPMoveR = true;
 }
 if(Game.getnewBackground().x <= Game.getFrameSize().getWidth() - (Game.getnewBackground().sx))
 {
 Game.getnewBackground().x = Game.getFrameSize().getWidth() - (Game.getnewBackground().sx);
 mToPMoveR = true;
 mToPMoveL = true;
 }
 }
 public void checkPlayerPosition()
 {
 if(Game.getPlayer().y <= 0)
 {
 Game.getPlayer().y = 0;
 }
 if(Game.getPlayer().y >= Game.getFrameSize().getHeight() - ((Game.getPlayer().sy * 2) - 4))
 {
 Game.getPlayer().y = Game.getFrameSize().getHeight() - ((Game.getPlayer().sy * 2) - 4);
 }
 if(Game.getPlayer().x <= 0)
 {
 Game.getPlayer().x = 0;
 }
 if(Game.getPlayer().x >= Game.getFrameSize().getWidth() - (Game.getPlayer().sx - 2))
 {
 Game.getPlayer().x = Game.getFrameSize().getWidth() - (Game.getPlayer().sx - 2);
 }
 }
 public void reverseMove()
 {
 if(Game.getPlayer().y == Game.getFrameSize().getHeight() / 2 && mToPMoveU && mToPMoveD)
 {
 mToPMoveD = false;
 mToPMoveU = false;
 }
 if(Game.getPlayer().y == Game.getFrameSize().getHeight() / 2 && mToPMoveD && mToPMoveU)
 {
 mToPMoveU = false;
 mToPMoveD = false;
 }
 if(Game.getPlayer().x == Game.getFrameSize().getWidth() / 2 && mToPMoveL && mToPMoveR)
 {
 mToPMoveL = false;
 mToPMoveR = false;
 }
 if(Game.getPlayer().x == Game.getFrameSize().getWidth() / 2 && mToPMoveR && mToPMoveL)
 {
 mToPMoveR = false;
 mToPMoveL = false;
 }
 }
}

Answer 1

The first thing I noticed is that you're lacking separation of concerns. Why does you Player class implement KeyListener? It should just represent the state of your player in the game without dealing with user input. That should be the concern of another class.

Why do you have public int SPEED = 3;? You should really pass this a parameter to the constructor (or to a modifier method). Public fields are almost always a bad idea. You usually don't want them so you can have a better control of who/how can modify them.

mToPMoveU, mToPMoveD, mToPMoveL, and mToPMoveR have a very poor naming. What about introducing an enum MoveDirection with Up, Down, Left, Right, (and maybe None)? That should help you make your code cleaner.

Why is image a public member?

What is the meaning of sx and sy? Their naming is cryptic and does not help me understand what is their purpose.

All the invocation to static methods in Game like Game.getnewBackground().y are a huge smell. Why do you need them? You should avoid almost at all static methods. Probably what you need is an public Movement update() method, that returns information about the movement performed by the player. It should be called by Game, which should also compute the new position regarding the current background. It does not make much sense to have it in Player. Ditto for checkMapPosition, checkPlayerPosition, and reverseMove invocations.

You want a different class to handle input and rendering. That class should contain the render, keyTyped, keyPressed, keyReleased methdods. They clearly don't belong to Player.

I hope that these comments will give you some ideas on how to structure better your code.

• tim: I just wrote an answer containing exactly the same points, so instead of posting it just a quick addition: dn, lt, rt could also be better named, as could checkPlayerPosition, which doesn't just check the position, it also corrects it, and reverseMove, which doesn't really reverse a move.
• Twiggy: Well thank you both for the suggestions. I was planning on putting most of that code in it's own class, this was mostly a test to see if i could get it working. One thing though, if I'm not supposed to use static methods to get the background and player, how am I supposed to move them?
• mariosangiorgio: You should have a Game class that holds references to instances of Player class. Game should know the status of the world and of the background. It should also take care of the update cycle. In Game.update() you need to call Player.update() and you have two ways to implement it. In the first you should obtain from it an object representing the movement and have Game process it. Alternatively you can pass the state of the world as a parameter to Player.update(state) and let it update its internal state. I think that the first solution is cleaner than the second.
• Twiggy: I do have a Game class that is the main logic of the program. So do you mean that if I call the movement object in Game that I won't need the static methods? Or do I use the variables I was using as parameters for the movement object?
• mariosangiorgio: Well, I think that it depends a bit on the game you are implementing and on what you need to do. As far as I understood your requirements, you probably need a Player class that holds information about the player. In this case I think we are only interested in its speed. Then you need a Game class that needs to keep track of the state of the world and another class that process inputs form the user. When you get an input from the user, you need to call an Update(action) method on Game. Action is something like MoveLeft, MoveRight, and so on.