Small refactoring example

This is a rather "bloggy" entry, but I want to share it with you. Of course this is simple stuff and totally uninteresting for the advanced jME guru. But maybe newbies can learn something from it, like I did.



Even if your class "works", you shouldn't stop improving it. With a little bit effort you can make your code so much cleaner and better. This is an easy example of a class I probably browsed through a hundred times and never took the time to take a closer look:



package jmetest.monkeymahjongg.game;

import com.jme.input.KeyInput;
import com.jme.input.controls.GameControl;
import com.jme.input.controls.GameControlManager;
import com.jme.input.controls.binding.KeyboardBinding;
import com.jme.math.Quaternion;
import com.jme.scene.Controller;

/**
 *
 * @author Pirx
 */
class CameraController extends Controller {
   private static final long serialVersionUID = 1L;

   private final static float MIN_ANGLE = 1.2f;
   private final static float MIN_DISTANCE = 35f;
   private final static float MAX_DISTANCE = 150f;
   private final static float SPEED = 2f;

   private float vAngle = 0;
   private float hAngle = 0;
   private float distance = 100;

   private GameControl left;
   private GameControl right;
   private GameControl up;
   private GameControl down;
   private GameControl forward;
   private GameControl backward;
       
        private CameraGameState gameState;

   public CameraController(CameraGameState gameState) {
            
                this.gameState = gameState;
           
      GameControlManager manager = new GameControlManager();
      left = manager.addControl("left");
      left.addBinding(new KeyboardBinding(KeyInput.KEY_LEFT));
      right = manager.addControl("right");
      right.addBinding(new KeyboardBinding(KeyInput.KEY_RIGHT));
      up = manager.addControl("up");
      up.addBinding(new KeyboardBinding(KeyInput.KEY_UP));
      down = manager.addControl("down");
      down.addBinding(new KeyboardBinding(KeyInput.KEY_DOWN));
      forward = manager.addControl("forward");
      forward.addBinding(new KeyboardBinding(KeyInput.KEY_PGUP));
      backward = manager.addControl("backward");
      backward.addBinding(new KeyboardBinding(KeyInput.KEY_PGDN));
   }


   public void update(float time) {
           
                //System.out.println("rot:" + gameState.getCameraRotationNode().getLocalRotation());
                //System.out.println("trans:" + gameState.getCameraDistanceNode().getLocalTranslation());
           
      float newHAngle = hAngle + SPEED * time
            * (right.getValue() - left.getValue());
      if (-MIN_ANGLE < newHAngle && newHAngle < MIN_ANGLE) {
         hAngle = newHAngle;
      }
      float newVAngle = vAngle + SPEED * time
            * (down.getValue() - up.getValue());
      if (-MIN_ANGLE < newVAngle && newVAngle < MIN_ANGLE) {
         vAngle = newVAngle;
      }
      gameState.getCameraRotationNode().setLocalRotation(new Quaternion(new float[] { vAngle,
            hAngle, 0f }));

      float newDist = distance + 20 * SPEED * time
            * (backward.getValue() - forward.getValue());
      if (MIN_DISTANCE < newDist && newDist < MAX_DISTANCE) {
         distance = newDist;
      }
      gameState.getCameraDistanceNode().setLocalTranslation(0, 0, distance);
   }

}



Today I finally realized how poorly this class was written. This is especially bad as the MonkeyMahjongg project is intended to help newbies, not to confuse them.

Hmmm, what is wrong here? First of all, we do the same crap for GameControls  again and again. Oh, and by the way, we can get the GameControls from the GameControlManager (that's the point of having names when adding it). Let's write a function for this:


    private void bind(String name, int... keys) {
        final GameControl control = manager.addControl(name);
        for (int key : keys) {
          control.addBinding(new KeyboardBinding(key));
        }
    }



Of course we need to have the GameControlManager as class member now, but we can throw the GameControl members away.

But somehow the Strings as controller names stink, so I replaced them by an enum:


enum Direction {LEFT, RIGHT, UP, DOWN, FORWARD, BACK};
...
    private void bind(Direction direction, int... keys) {
        final GameControl control = manager.addControl(direction.name());
        for (int key : keys) {
          control.addBinding(new KeyboardBinding(key));
        }
    }



Hmmm, how we get the values of a certain game control? OK, new function:


    private float value(Direction direction) {
        return manager.getControl(direction.name()).getValue();
    }



Now to the update function. It works, but it's a mess. We calculate temporary values (like newHAngle), look if they are in range, and then copy them to the original value (like hAngle). That's stupid. I remembered there is a clamp function in FastMath, tried it out and the temporary variables were gone:


//too lazy to type
import static com.jme.math.FastMath.clamp;
import static jmetest.monkeymahjongg.game.CameraController.Direction.*;
...
        float way = SPEED * time; //we need this several times
        hAngle = clamp(hAngle + way * (value(RIGHT) - value(LEFT)), -MIN_ANGLE, MIN_ANGLE);



That's much better! Now the quaternion: It has an ugly constructor using a float array. Why? A short look in the jME source told me that it just sets the Euler angles using Quaternion.fromAngles, nothing else. So we can do this instead:


gameState.getCameraRotationNode().getLocalRotation().fromAngles(vAngle, hAngle, 0f);



Nice, no more Quaterion object creation needed.

All together, the class looks now like this:


package jmetest.monkeymahjongg.game;

import com.jme.input.controls.GameControl;
import com.jme.input.controls.GameControlManager;
import com.jme.input.controls.binding.KeyboardBinding;
import com.jme.scene.Controller;

import static com.jme.input.KeyInput.*;
import static com.jme.math.FastMath.clamp;
import static jmetest.monkeymahjongg.game.CameraController.Direction.*;

/**
 *
 * @author Pirx
 */
class CameraController extends Controller {

    private static final long serialVersionUID = 1L;
   
    enum Direction {LEFT, RIGHT, UP, DOWN, FORWARD, BACK};
   
    private final static float MIN_ANGLE = 1.2f;
    private final static float MIN_DISTANCE = 35f;
    private final static float MAX_DISTANCE = 150f;
    private final static float SPEED = 2f;
    private final GameControlManager manager = new GameControlManager();
    private final CameraGameState gameState;
   
    private float vAngle = 0;
    private float hAngle = 0;
    private float distance = 100;

    public CameraController(CameraGameState gameState) {
        this.gameState = gameState;

        bind(LEFT, KEY_LEFT);
        bind(RIGHT, KEY_RIGHT);
        bind(UP, KEY_UP);
        bind(DOWN, KEY_DOWN);
        bind(FORWARD, KEY_PGUP, KEY_ADD);
        bind(BACK, KEY_PGDN, KEY_SUBTRACT);
    }
   
    private void bind(Direction direction, int... keys) {
        final GameControl control = manager.addControl(direction.name());
        for (int key : keys) {
          control.addBinding(new KeyboardBinding(key));
        }
    }
   
    private float value(Direction direction) {
        return manager.getControl(direction.name()).getValue();
    }

    public void update(float time) {
        final float way = SPEED * time;
        hAngle = clamp(hAngle + way * (value(RIGHT) - value(LEFT)), -MIN_ANGLE, MIN_ANGLE);
        vAngle = clamp(vAngle + way * (value(DOWN) - value(UP)), -MIN_ANGLE, MIN_ANGLE);
        gameState.getCameraRotationNode().getLocalRotation().fromAngles(vAngle, hAngle, 0f);
        distance = clamp(distance + 20 * way * (value(BACK) - value(FORWARD)),
              MIN_DISTANCE, MAX_DISTANCE);
        gameState.getCameraDistanceNode().setLocalTranslation(0, 0, distance);
    }
}



That's short! And quite readable. The static imports are certainly a matter of taste (note that I had to made the Direction enum "package private" in order to get the static import working), but the enum itself is not: I got bitten often enough when using Strings as name tags. Just don't do it.

Another thing I should mention is that I made the member variables final when their references don't change. This is always good idea: For example you can see with one look that manager will be always the same old instance for the whole lifetime of the CameraController object. You can't make any mistakes when you try to add a final modifier, as the compiler (or the IDE) will point out when it is not possible to use it.

So what is the message here? Is there any? In fact there are several ones:
- DRY = Don't repeat yourself [A.k.a: Three times is a pattern] (GameControls)
- Know your libraries (clamp, Quaternion)
- static import can save you a lot of typing
- don't use Strings as name tags (i.e. use enums instead)
- make member variables final if possible

Refactoring your classes makes them more readable, more robust and maybe faster. You can also learn how to write new classes better from the very beginning.

I hope you enjoyed (and not only my funny English)!

Great post Landei! There is nothing better than concise code when you're trying to understand it.

i didn't know about static imports yet :slight_smile:

i need to refactor all the time too, but often it gets more messy than before if you try to refactor only small bits of code. :slight_smile:

Its important to keep the whole project consistent.

Thanks Landei :slight_smile: I am ashamed to admit but a lot of the stuff you wrote was quite new to me…

You should consider converting this into a little tutorial and putting it on the wiki.  This is about as close as exists to a tutorial on GameControls…granted that's my fault, but even so. :slight_smile:

I love the amount of freely available Java and JME code available.  It makes it very easy to learn new approaches, to advance/progress. 



Sadly at work I have to work with code that was written recently (within 5 years) and code that has been written nearly 20 years ago.  We're basically standing still, reusing the same old solutions to problems; "I solved this type of problem 15 years ago and this is how I did it then and this is how I'll do it again today".  There's safety in those age old solutions, they are known, familiar.  Trying to implement modern concepts (unit testing, creating reusable procedures rather than putting all the code within windows/forms) is extremely difficult.



Refactoring is a good approach.  Another is the proper use of variables.  I sometimes have to deal with:  iTot, iTot2, iTot3, iTotal, iAmount, etc.  There's also a lack of comment, to describe the goal of the code.  Finally there's the problem where a bit of code is thought to be slow and is optimized to the point where the resulting code becomes so cryptic that making changes becomes quite hazardous.

at work, we have often do deal with nested c for loops and Variable names like nI, nII, nIII , or just plain i :slight_smile:



As we are on that topic, i like the book Code Complete, its not really about a specific language, but good programming style in general.

Finally there's the problem where a bit of code is thought to be slow and is optimized to the point where the resulting code becomes so cryptic that making changes becomes quite hazardous.


i once won a java minesweeper solver contest. my first version was readable and solved a given example field in 7 seconds. the final version is rather wacky, but the time needed to solve the field isn't even measureable using System.currentTimeMillis() :D
there's just no way anyone besides me will ever be able to even look at the code without getting dizzy.

Another refactoring example. From this application:


public class HelloWorld {
   public static void main(String... args) {
      System.out.println("Hello World!");
   }
}



I got with a little refactoring effort this:


public class HelloWorld {
  private HelloWorld(){HelloWorld();};
  private void HelloWorld() {helloWorld();}
  private void helloWorld(String HelloWorld) {Helloworld.helloWorld.out.print(HelloWorld);}
  private void helloWorld() { char[] HelloWorld = new char['r'];
    final int[] helloWorld = new int[]{'-'-'-'};
    switch (helloWorld['+'-'+']) {
        case '-'-'-' :
        case 'H' : HelloWorld[helloWorld['+'-'+']++] =
              0110;
        case 'e' : HelloWorld[helloWorld['-'-'-']++] =
              '/'/'/' + 'n' * 'n';
        case 'l' : HelloWorld[helloWorld['!'-'!']++] =
              011 + 011 + 011 << 2;
        case 'L' : HelloWorld[helloWorld['-'-'-']++] =
              HelloWorld['/'/'/' << 1 ];
        case 'o' : HelloWorld[helloWorld['_'-'_']++] =
              (char)('n' + HelloWorld['/'/'/']);
        case ' ' : HelloWorld[helloWorld['-'-'-']++] =
              '!' - '/'/'/';
        case 'W' : HelloWorld[helloWorld['o'-'o']++] =
              ('n' << 1) + (1 << 6) + 'u0003';
        case 'O' : HelloWorld[helloWorld['_'-'_']++] =
              HelloWorld['/'/'/' << 2];
        case 'r' : HelloWorld[helloWorld['-'-'-']++] =
              '@' + '2';
        case '1' : HelloWorld[helloWorld['''-''']++] =
              HelloWorld['/'/'/' << 1];
        case 'd' : HelloWorld[helloWorld['\'-'\']++] =
              '2' << 1;
        case '!' : HelloWorld[helloWorld['/'-'/']++] =
              'r' + ('n' << 1);
        case 'n' : HelloWorld[helloWorld['-'-'-']++] =
              '(' >> 2; helloWorld(new String(HelloWorld)); }
  }
  private static class Helloworld {
  private static System helloWorld = HelloWorld.helloworld();
  private static System helloWorld() {return null;}}
  private static System helloworld() {return Helloworld.helloWorld();}
  private static HelloWorld HelloWorld = new HelloWorld(){{Helloworld.helloWorld.exit(0);}};
}



Granted, it's a little bit longer, but readability counts and not the number of lines...

You should consider converting this into a little tutorial and putting it on the wiki.  This is about as close as exists to a tutorial on GameControls....granted that's my fault, but even so.


http://www.jmonkeyengine.com/wiki/doku.php?id=gamecontrol_basics

Thanks Landei :slight_smile: I like it

Thanks Landei!

Maybe we should even start a section on "programming techniques" or something on the wiki to host the excellent comments on refactoring in your first post and hopefully more insights from others in the future.

readability counts


my IDE is telling me your code is very, very evil. should i spank it?

my IDE is telling me your code is very, very evil. should i spank it?


You need an IDE to see that?

But honestly, of course I know that my HelloWorld example is evil:
- it uses no generics
- it uses no enums
- it uses no reflection
- it uses no patterns

Maybe I'll update it accordingly.

BTW: I found it more effective to sentence someone to several years of GridBagLayout instead of spanking, because that really hurts.

You need an IDE to see that?


i suspected it, but IDEA told me for sure that your code is evil. it's giving me exactly 62 warnings.
btw, how to start it? there is no main method.