Take the 2-minute tour ×
Code Review Stack Exchange is a question and answer site for peer programmer code reviews. It's 100% free, no registration required.

My code first creates a Point object. Then it creates a 25 x 25 array of ASCII in a string where _ marks a miss and the point's symbol (in this case "X") marks a hit. Then, the user can move the point with the arrow keys and it rerenders.

This code is probably really badly written and terrible, but it works. I need some constructive feedback, and some tips on optimizing it as it kinda flickers whenever I move the x with the arrow keys.

import java.awt.*;
import java.awt.event.*;
import javax.swing.JFrame;

public class _Newmain extends Canvas {
    private static int renderSizeX = 25;
    private static int renderSizeY = 25;

    private point player = new point(10,10,'X');
    public _Newmain() {
        setSize(new Dimension(500, 500));
        addKeyListener(new KeyAdapter(){
                @Override
                public void keyPressed(KeyEvent evt) {
                    moveIt(evt);
                }
            });
    }

    public void paint(Graphics g) {
        String text = m2.outputScreen(new point[]{player});
        int x = 20; int y = 20;
        for (String line : text.split("\n"))
            g.drawString(line, x, y += g.getFontMetrics().getHeight());

    }

    public void moveIt(KeyEvent evt) {
        switch (evt.getKeyCode()) {
            case KeyEvent.VK_DOWN:
            player.moveY(1);
            break;
            case KeyEvent.VK_UP:
            player.moveY(-1);
            break;
            case KeyEvent.VK_LEFT:
            player.moveX(-1);
            break;
            case KeyEvent.VK_RIGHT:
            player.moveX(1);
            break;
        }

        repaint();
    }

    public static void main(String[] args) {
        JFrame frame = new JFrame("TEXT BASED GAME WRITTEN BY 10REPLIES");
        frame.setDefaultCloseOperation(JFrame.EXIT_ON_CLOSE);
        _Newmain ex = new _Newmain();
        frame.getContentPane().add(ex);
        frame.pack();
        frame.setResizable(false);
        frame.setVisible(true);
        ex.requestFocus();

    }

    public static int windowY(){
        return renderSizeY;
    }

    public static int windowX(){
        return renderSizeX;
    }
}

ASCII generating class

public class m2
{
    // instance variables - replace the example below with your own
    public static void render(point[] input){
        System.out.print("\f");//Clears screen
        for(int y = 0; y < _Newmain.windowY(); y++){
            for(int x = 0; x < _Newmain.windowX(); x++){
                char found = ' ';

                for(int i = 0; i < input.length;i++){
                    if( input[i].getY() == y && input[i].getX() == x ){//checks to see if list of points contains a point at current x and y position
                        found = input[i].getChar();
                    }
                }
                System.out.print(found);//prints char if found else prints space
            }
            System.out.print("\n");
        }

    }
        public static String outputScreen(point[] input){
        String output ="";
        for(int y = 0; y < _Newmain.windowY(); y++){
            for(int x = 0; x < _Newmain.windowX(); x++){
                char found = '_';

                for(int i = 0; i < input.length;i++){
                    if( input[i].getY() == y && input[i].getX() == x ){//checks to see if list of points contains a point at current x and y position
                        found = input[i].getChar();
                    }
                }
                output=output+found;
            }
            output=output+"\n";
        }
        return output;
    }

}

Point class

public class point
{
    int x, y;
    char letter;
    public point(int inX, int inY, char letterRepresent)
    {
        x = inX;
        y = inY;
        letter=letterRepresent;
    }

    public char getChar(){return letter;}
    public int getX(){return x;}
    public int getY(){return y;}
    public void moveX(int ammount){x+=ammount;}
    public void setX(int location){x=location;}
    public void moveY(int ammount){y+=ammount;}
    public void setY(int location){y=location;}
}
share|improve this question

1 Answer 1

Class Names

I will address a few problems in no particular order.

public class _Newmain extends Canvas {
public class m2

These are both really bad names for classes. In Java the standard is to use PascalCase for class names. So instead of _Newmain you would say NewMain. However, it is most important to make sure that your class names provide some additional information about what the class actually represents, or what its purpose is.

Magic Numbers

private point player = new point(10,10,'X');
setSize(new Dimension(500, 500));
int x = 20; int y = 20;
player.moveY(1);

All of these are examples of magic numbers in your code. Now, I am not saying that you absolutely have to declare a variable for every single number everywhere in your code, but I will say that it always makes the code more clear, even if it can sometimes be a little verbose. It might make sense to you what 10 and 10 are when initializing a new player, but it might not make sense to someone else reading your code. None of these examples are really bad (although the x = 20 y = 20 is the worst offender in my opinion), but it is a good idea to get into the habit of always initializing variables with descriptive names. This will make the code more self documenting and easier to understand.

White Space and Indentation

I would give yourself a bit more room to breathe after class declarations:

public class _Newmain extends Canvas {
    private static int renderSizeX = 25;
    private static int renderSizeY = 25;

    private point player = new point(10,10,'X');
    public _Newmain() {

This would become:

public class _Newmain extends Canvas {

    private static int renderSizeX = 25;
    private static int renderSizeY = 25;

    private point player = new point(10,10,'X');

    public _Newmain() {

I think that this gives you a better understanding at just a glance what is static, what the fields are for the class, and where the class constructor starts.

public static String outputScreen(point[] input){ 

This line should not be indented so far, and I would also put one line of white space between method declarations in the class.

public void moveX(int ammount){x+=ammount;}
public void setX(int location){x=location;}
public void moveY(int ammount){y+=ammount;}
public void setY(int location){y=location;}

This is definitely a matter of opinion, but I think that single line function declarations are harder to understand. You also have a typo here with two m's in amount.

String Construction

String output ="";
output=output+found;
output=output+"\n";

If possible, I would use StringBuilder instead of simply assigning the results of adding the string in this way. I think that it makes the intention of the code more clear.

I don't think that you would actually see a performance increase in the lines I have pasted above, but any time that you are going to append more than one value at once, you will definitely see better performance. When you add two strings with +, two different instances of StringBuilder are being created behind the scenes, so when you manually append with StringBuilder, you are only using one instance. See this answer about string concatenation for more information.

Imports

import java.awt.*;
import java.awt.event.*;

I would recommend that you never use .* for importing, but rather always import only exactly what is needed in this class. This might add a lot more lines to the top of the file, but it is more efficient during compilation because it will be linking to less files, and it will also be more clear from looking at the imports which libraries/classes are used in this one.

share|improve this answer
    
How do I know what libraries I need to import? –  10 Replies Apr 14 at 0:25
    
Also, how can I fix the flickering? –  10 Replies Apr 14 at 0:26

Your Answer

 
discard

By posting your answer, you agree to the privacy policy and terms of service.

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