4
\$\begingroup\$

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();
}
\$\endgroup\$
  • \$\begingroup\$ Welcome to CR! Are you on Java 8? \$\endgroup\$ – h.j.k. Mar 31 '16 at 3:35
  • 1
    \$\begingroup\$ Pls, update your code with full class code if possible \$\endgroup\$ – Andriy Kryvtsun Apr 1 '16 at 21:29
  • 1
    \$\begingroup\$ This question is incomplete. To help reviewers give you better answers, please add sufficient context to your question. The more you tell us about what your code does and what the purpose of doing that is, the easier it will be for reviewers to help you. See also this meta question. \$\endgroup\$ – Vogel612 Apr 1 '16 at 23:41
  • \$\begingroup\$ Please describe the code. It is much easier to understand code when its purpose and function are detailed. \$\endgroup\$ – motoku Apr 2 '16 at 1:22
2
\$\begingroup\$

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 and HEIGHT instead of Width and Height

  • Leave some space between operators when you do calculations: return Sys.getTime()*1000/Sys.getTimerResolution(); is rather unreadable compared to return 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. Use JList<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 be ContextAttributes.

  • Don't number your variables. That's semantically unhelpful. What's the difference between frame, frame2 and frame3?? How will you understand the difference if you don't see the full code?
    Same goes for text and text2.

  • Don't name things after their type. JList list is incredibly unuseful. It provides no information whatsoever

\$\endgroup\$

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.