2
\$\begingroup\$

I try to intercept some OpenGL calls for testing my rendering classes. Reflection is used for replacing the OpenGL backend.

I feel this class is badly written and I need advices for refactoring it.

package fr.meuns.opengl;

import java.lang.reflect.InvocationTargetException;
import java.lang.reflect.Method;
import java.nio.FloatBuffer;

public class GL
{
  private static class MethodSignature
  {
    String name;
    Class< ? >[] parameters;

    public MethodSignature( String name, Class< ? >... parameters )
    {
      this.name = name;
      this.parameters = parameters;
    }
  }

  private final static MethodSignature[] METHOD_SIGNATURES = new MethodSignature[] {
    new MethodSignature( "glGenBuffers" ),
    new MethodSignature( "glDeleteBuffers", int.class ),
    new MethodSignature( "glBindBuffer", int.class, int.class ),
    new MethodSignature( "glBufferData", int.class, FloatBuffer.class, int.class ),
    new MethodSignature( "glGetBufferSubData", int.class, long.class, FloatBuffer.class )
    ...
  };

  private final static String[] CONSTANT_NAMES = new String[] {
    "GL_ARRAY_BUFFER",
    "GL_ELEMENT_ARRAY_BUFFER",
    "GL_STATIC_DRAW"
    ...
  };

  public static int GL_ARRAY_BUFFER;
  public static int GL_ELEMENT_ARRAY_BUFFER;
  public static int GL_STATIC_DRAW;
  ...

  public static Method _glGenBuffers;
  public static Method _glDeleteBuffers;
  public static Method _glBindBuffer;
  public static Method _glBufferData;
  public static Method _glGetBufferSubData;
  ...

  public static int glGenBuffers()
  {
    try
    {
      return (int)_glGenBuffers.invoke( null );
    }
    catch( IllegalAccessException | IllegalArgumentException | InvocationTargetException e )
    {
      e.printStackTrace();
      return 0;
    }
  }

  public static void glDeleteBuffers( int buffer )
  {
    try
    {
      _glDeleteBuffers.invoke( null, buffer );
    }
    catch( IllegalAccessException | IllegalArgumentException | InvocationTargetException e )
    {
      e.printStackTrace();
    }
  }

  public static void glBindBuffer( int target, int buffer )
  {
    try
    {
      _glBindBuffer.invoke( null, target, buffer );
    }
    catch( IllegalAccessException | IllegalArgumentException | InvocationTargetException e )
    {
      e.printStackTrace();
    }
  }

  public static void glBufferData( int target, FloatBuffer data, int usage )
  {
    try
    {
      _glBufferData.invoke( null, target, data, usage );
    }
    catch( IllegalAccessException | IllegalArgumentException | InvocationTargetException e )
    {
      e.printStackTrace();
    }
  }

  public static void glGetBufferSubData( int target, long offset, FloatBuffer data )
  {
    try
    {
      _glGetBufferSubData.invoke( null, target, offset, data );
    }
    catch( IllegalAccessException | IllegalArgumentException | InvocationTargetException e )
    {
      e.printStackTrace();
    }
  }

  public ...

  public static void useClass( Class< ? > glClass )
  {
    for( String name : CONSTANT_NAMES )
      try
      {
        GL.class
          .getField( name )
          .set( null, glClass.getField( name ).get( null ) );
      }
      catch( IllegalArgumentException | IllegalAccessException | NoSuchFieldException | SecurityException e1 )
      {
        e1.printStackTrace();
      }

    for( MethodSignature signature : METHOD_SIGNATURES )
      try
      {
        GL.class
          .getField( "_" + signature.name )
          .set( null, glClass.getDeclaredMethod( signature.name, signature.parameters ) );
      }
      catch( IllegalArgumentException | IllegalAccessException | NoSuchFieldException | SecurityException | NoSuchMethodException e )
      {
        e.printStackTrace();
      }
  }
}

Initialization for rendering using LWJGL :

import org.lwjgl.opengl.GL11;
import org.lwjgl.opengl.GL15;
import org.lwjgl.opengl.GL20;
...

GL.useClass( GL11.class );
GL.useClass( GL15.class );
GL.useClass( GL20.class );
...

Initialization for testing :

import fr.meuns.opengl.GLMock;
...

GL.useClass( GLMock.class );
...
\$\endgroup\$
  • \$\begingroup\$ I can elaborate if necessary. \$\endgroup\$ – sylvain May 14 '13 at 21:05
2
\$\begingroup\$
public class GL

Your class name should really be a more descriptive noun.

private static class MethodSignature

You actually have no need for this private static class since Reflection gives you direct access to methods and method signatures, as you'll see in my modified version of your code below.

String name;
Class<?>[] parameters;

When you create a class, you should always explicitly declare whether a field is public, protected, or private.

private final static MethodSignature[] METHOD_SIGNATURES = new MethodSignature[] { //...
private final static String[] CONSTANT_NAMES = new String[] { //...

Again, you don't need either of these since you have Reflection, which gives you access to methods and fields.

public static int GL_ARRAY_BUFFER;
public static int GL_ELEMENT_ARRAY_BUFFER;
public static int GL_STATIC_DRAW;

public static Method _glGenBuffers;
public static Method _glDeleteBuffers;
public static Method _glBindBuffer;
public static Method _glBufferData;
public static Method _glGetBufferSubData;

These are all very odd variable names. In general, NAMES_IN_CAPS are constants (private static final). The beginning underscore is also a paradigm that isn't really Java-esque (and to my knowledge in other languages it is generally used for private-like fields).

public static int glGenBuffers() {

Your method names are also oddly named for Java. In general, Java method names should be actions and verb phrases. (For example, getGLGenBuffers())

catch(IllegalAccessException | IllegalArgumentException | InvocationTargetException e)

No need for these lengthy catch declarations since they all do the same thing and all you're doing is printing the stack trace. catch(Exception e) will do and make your code more sensible.

public static void useClass(Class<?> glClass)

This entire method will be refactored below. But basically you don't need two for() loops, since you're just looping through the class's fields. This also eliminates the need for the overarching constants for the field names and the method signatures, as you'll see.

I cut your code length by about half by doing all of the above, and I think it's generally more readable and understandable this way as well.

import java.lang.reflect.Field;
import java.lang.reflect.Method;
import java.nio.FloatBuffer;

public class OpenGLInterceptor {    

    public static int GL_ARRAY_BUFFER;
    public static int GL_ELEMENT_ARRAY_BUFFER;
    public static int GL_STATIC_DRAW;
    //...

    public static Method _glGenBuffers;
    public static Method _glDeleteBuffers;
    public static Method _glBindBuffer;
    public static Method _glBufferData;
    public static Method _glGetBufferSubData;
    //...

    public static int invokeGenBuffers() {
        try {
            return (Integer)_glGenBuffers.invoke(null);
        }
        catch(Exception e) {
            e.printStackTrace();
            return 0;
        }
    }

    public static void invokeDeleteBuffers(int buffer) {
        try {
            _glDeleteBuffers.invoke(null, buffer);
        }
        catch(Exception e){
            e.printStackTrace();
        }
    }

    public static void invokeBindBuffer(int target, int buffer) {
        try {
          _glBindBuffer.invoke( null, target, buffer );
        }
        catch(Exception e) {
          e.printStackTrace();
        }
    }

    public static void invokeBufferData(int target, FloatBuffer data, int usage) {
        try {
            _glBufferData.invoke(null, target, data, usage);
        }
        catch(Exception e) {
            e.printStackTrace();
        }
    }

    public static void invokeGetBufferSubData(int target, long offset, FloatBuffer data) {
        try {
            _glGetBufferSubData.invoke( null, target, offset, data );
        }
        catch(Exception e) {
            e.printStackTrace();
        }
    }

    public static void useClass(Class<?> glClass) {
        for(Field field : GL.class.getDeclaredFields()) {
            try {
                if(field.getName().contains("_")) {
                    field.set(null, glClass.getDeclaredMethod(field.getName(), ((Method)field.get(null)).getParameterTypes()));
                }
                else {
                    field.set(null, glClass.getField(field.getName()).get(null));
                }
            }
            catch(Exception e) {
                e.printStackTrace();
            }
        }
    }

}
\$\endgroup\$
  • \$\begingroup\$ A lot of good ideas, thank you. Tomorrow I will take more times to think about you post. \$\endgroup\$ – sylvain Oct 2 '13 at 23:17
  • \$\begingroup\$ Renaming gl* to invoke* implies a lot of renaming in the existing engine code and also reduce readability. \$\endgroup\$ – sylvain Oct 4 '13 at 21:18
  • \$\begingroup\$ @sylvain Why would you have to rename the existing engine code? From what you said, your code is just to intercept what that code is doing, so it should be entirely separate. I also completely disagree with you about the readability bit, but of course that's subjective. \$\endgroup\$ – asteri Oct 4 '13 at 21:31
  • \$\begingroup\$ Because I want to instrument the whole engine. Rendering classes use the OpenGLInterceptor (or GL in case). For running application I just use classes provided by LWJGL but for testing I can use a mock or log class. \$\endgroup\$ – sylvain Oct 5 '13 at 13:55
  • \$\begingroup\$ When you create a class, you should always explicitly declare whether a field is public, protected, or private. - But not declaring it any of the three results in a fourth choice, package-protected, for which there is no explicit declaration. If the OP actually wants package-protected fields, not adding any explicit declaration was his only choice. \$\endgroup\$ – mkl Nov 1 '13 at 21:44

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.