Problem
I would like someone to review my code and maybe give me some advice to maintain it because I’m still a beginner.
public static boolean NewTerrainCamPos = false;
public static String textVal;
public static String textVal2;
public static String resiveTex = "1";
public static String resiveTex2;
public static final int Width = 1000;
public static final int Height = 720;
public static final int FPS_CAP = 120;
private static long lastFrameTime;
private static float delta;
public static void createDisplay(){
ContextAttribs attribs = new ContextAttribs(3,2).withForwardCompatible(true).withProfileCore(true);
try {
Canvas openglSurface = new Canvas();
JFrame frame = new JFrame();
frame.setDefaultCloseOperation(JFrame.EXIT_ON_CLOSE);
frame.setLayout(new BorderLayout());
//.............................
JMenuBar menuBar = new JMenuBar();
JMenu terrain = new JMenu("Terrain");
menuBar.add(terrain);
menuBar.add(terrain);
JMenuItem exit = new JMenuItem("Exit");
JMenuItem newTerrain = new JMenuItem("add Terrain");
JMenuItem editTerrain = new JMenuItem("Edit Terrain");
newTerrain.addActionListener(new ActionListener(){
public void actionPerformed(ActionEvent e) {
NewTerrainCamPos = true;
JFrame frame2 = new JFrame();
frame2.setVisible(true);
frame2.setSize(300, 300);
//...............................
GridLayout experimentLayout = new GridLayout(3,2);
frame2.setLayout(experimentLayout);
//.....................................
JLabel xCord = new JLabel("XCoords: ");
JLabel zCord = new JLabel("ZCoords: ");
JTextField text = new JTextField();
JTextField text2 = new JTextField();
resiveTex2 = text2.getText();
text.getDocument().addDocumentListener(new DocumentListener() {
@Override
public void insertUpdate(DocumentEvent de) {
resiveTex = text.getText();
}
@Override
public void removeUpdate(DocumentEvent de) {
resiveTex = text.getText();
}
@Override
public void changedUpdate(DocumentEvent de) {
//plain text components don't fire these events
}
});
text2.getDocument().addDocumentListener(new DocumentListener() {
@Override
public void insertUpdate(DocumentEvent de) {
resiveTex2 = text2.getText();
}
@Override
public void removeUpdate(DocumentEvent de) {
resiveTex2 = text2.getText();
}
@Override
public void changedUpdate(DocumentEvent de) {
//plain text components don't fire these events
}
});
JButton createTerrain = new JButton("CreateTerrain");
createTerrain.addActionListener(new ActionListener(){
TIDF terrainFileID;
public void actionPerformed(ActionEvent a){
NewTerrainCamPos = false;
textVal = text.getText();
textVal2 = text2.getText();
TIDF.terrainIDFile();
}
});
frame2.add(xCord);
frame2.add(text);
frame2.add(zCord);
frame2.add(text2);
frame2.add(createTerrain);
}
});
editTerrain.addActionListener(new ActionListener(){
public void actionPerformed(ActionEvent e) {
JFrame frame3 = new JFrame();
frame3.setVisible(true);
frame3.setSize(300, 300);
//......................................
GridLayout experimentLayout = new GridLayout(3,2);
frame3.setLayout(experimentLayout);
//......................................
JButton select = new JButton("Select");
String terrainLocList[] =
{
"Item 1",
"Item 2",
"Item 3",
"Item 4"
};
JList list = new JList(terrainLocList);
list.setSelectionMode(ListSelectionModel.SINGLE_INTERVAL_SELECTION);
list.setVisibleRowCount(-1);
frame3.add(list);
frame3.add(select);
}
});
terrain.add(newTerrain);
terrain.add(editTerrain);
terrain.add(exit);
frame.setJMenuBar(menuBar);
//.........................................
frame.setSize(1100, 1000);
frame.add(openglSurface);
frame.setVisible(true);
openglSurface.setSize(1000, 720);
Display.setParent(openglSurface);
Display.setDisplayMode(new DisplayMode(Width, Height));
Display.create(new PixelFormat(), attribs);
frame.setTitle("Game editor 0.1");
} catch (LWJGLException e) {
e.printStackTrace();
}
GL11.glViewport(0, 0, Width, Height);
lastFrameTime = getCurrentTime();
}
public static boolean Returnboolean(){
return NewTerrainCamPos;
}
public static String getTex1() {
return textVal;
}
public static String getTex2(){
return textVal2;
}
public static String getTexupdate(){
return resiveTex;
}
public static String getTexupdate2(){
return resiveTex2;
}
public static void updateDisplay(){
Display.sync(FPS_CAP);
Display.update();
long currentFrameTime = getCurrentTime();
delta = (currentFrameTime - lastFrameTime)/1000f;
lastFrameTime = currentFrameTime;
}
public static float getFrameTimeSeconds(){
return delta;
}
public static void closeDisplay(){
Display.destroy();
}
private static long getCurrentTime(){
return Sys.getTime()*1000/Sys.getTimerResolution();
}
Solution
Nitpicks:
-
Your indentation isn’t quite correct. It seems to have too many levels in some places, and it’s incorrect here:
try { Canvas openglSurface = new Canvas(); JFrame frame = new JFrame();
-
static final
fields should always be SHOUT_CASE.WIDTH
andHEIGHT
instead ofWidth
andHeight
-
Leave some space between operators when you do calculations:
return Sys.getTime()*1000/Sys.getTimerResolution();
is rather unreadable compared toreturn Sys.getTime() * 1000 / Sys.getTimerResolution();
-
Your capitalizaion between “Edit Terrain” and “add Terrain” is different. I suggest you stick to Title Case
-
Don’t use Rawtypes.
JList
should be qualified with a generic type. UseJList<String>
instead.
Access and Scope
All the fields declared public probably can (and should) be private. You shouldn’t be using them outside this class. This means you should make them inaccessible -> private.
Aside from that you’re overusing the static context. I’m feeling that I sound like a broken record, but… Avoid static like the plague. It goes against good OOP principles to use static fields for anything but actual constants, so don’t do it.
This advice applies to all of the code you have here except FPS_CAP
, WIDTH
and HEIGHT
.
Naming
-
Don’t shorthand names.
ContextAttribs
should beContextAttributes
. -
Don’t number your variables. That’s semantically unhelpful. What’s the difference between
frame
,frame2
andframe3
?? How will you understand the difference if you don’t see the full code?
Same goes fortext
andtext2
. -
Don’t name things after their type.
JList list
is incredibly unuseful. It provides no information whatsoever