0
\$\begingroup\$

i got a college project and i'm trying to create a map for my text-based game in Java. Me and my colleague* created this code:

public class Mapsystem2 {

//δημιουργία των πινάκων maps
static int[][] map1= { {0,0,0,0,0,0,3,3,3,3,0,0,0,0,0,0},{0,0,0,0,0,0,1,1,1,1,0,0,0,0,0,0},{0,3,3,0,0,0,1,1,1,1,1,1,1,1,1,1},{0,1,1,0,0,0,1,1,1,1,1,1,1,1,1,1},{0,1,1,1,1,1,1,1,1,1,2,2,2,2,1,1},{0,1,1,1,1,1,1,1,1,1,2,2,2,2,1,1},{0,0,0,0,0,0,1,1,1,1,1,1,1,1,1,1},{0,0,0,0,0,0,1,1,1,1,1,1,1,1,1,1},{0,0,0,0,0,0,1,1,2,2,0,0,0,0,0,0},{3,1,1,1,1,1,1,1,2,2,0,0,0,0,0,0},{3,1,1,1,1,1,1,1,2,2,0,0,0,0,0,0},{0,0,0,0,0,0,1,1,2,2,1,1,1,1,1,3},{0,0,0,0,0,0,1,1,2,2,1,1,1,1,1,3},{0,0,0,0,0,0,1,1,2,2,0,0,0,0,0,0},{0,0,0,0,0,0,1,1,1,1,0,0,0,0,0,0},{0,0,0,0,0,0,1,1,1,1,0,0,0,0,0,0}};
static int[][] map2= { {2,2,1,2,2,2,2,2,2,2,1,1},{2,2,1,2,2,2,2,2,2,2,1,1},{2,2,0,0,1,1,0,0,0,0,0,0},{2,2,0,0,1,1,0,0,0,0,0,0},{1,1,0,0,1,1,1,1,0,0,4,4},{1,1,0,0,2,2,2,2,0,0,1,1},{2,2,0,0,2,2,2,2,0,0,1,1},{2,2,0,0,2,2,2,2,0,0,2,2},{1,1,0,0,0,0,1,1,1,1,2,2},{1,1,0,0,0,0,1,1,1,1,2,2}};


static int tileCheck(int[] playerPos)

    {
     int x= playerPos[0];
     int y= playerPos[1];
     int a=0;
        //loadfile

      /**
        try (BufferedReader br = new BufferedReader(new FileReader("monsters.txt"))) {
         String sCurrentLine;

         while ((sCurrentLine = br.readLine()) != null) {
            System.out.println(sCurrentLine);
         }
      } catch (IOException e) {
         e.printStackTrace();
      }
      */
    int[][] map = Mapsystem2.map1;
    if(playerPos[0]<map.length || playerPos[0]>0 ||playerPos[1]<map.length || playerPos[1]>0)
    {
        a=1;
    }
    //if ( map[x][y]==2)
    //{
        //εδω θα καλουμε τη συναρτηση του battle system για να βγαινουν τα τερατα
    //}

    if ( map[x][y]==0)
    {
        System.out.println("you cannot go there");
        a=0; 
    }
    else if ( map[x][y]==1)
    {
        a=1;
    }
    /*else if ( map[x][y]==3)
    {
       //εδω θα αλλαζει το map
    }*/
    return(a);
    }


static int[] movement(String d,int[] playerPos)
    {

        int check=1;
            if ("north".equals(d))
            {
                check=tileCheck(playerPos);
                if(check==1)
                {
                    playerPos[1]= playerPos[1]-1;
                }
                else
                {System.out.println("You are getting out of the level");
                }
            }
            else if("south".equals(d))
            {
                check=tileCheck(playerPos);
                if(check==1)
                {
                    playerPos[1]=playerPos[1]+1;
                }
                else
                {System.out.println("You are getting out of the level");
                }
            }
            else if("west".equals(d))
            {
                check=tileCheck(playerPos);
                if(check==1)
                {
                    playerPos[0]=playerPos[0]-1;
                }
                else
                {System.out.println("You are getting out of the level");
                }
            }
            else if("east".equals(d))
            {
                check=tileCheck(playerPos);
                if(check==1)
                {
                    playerPos[0]=playerPos[0]+1;
                }
                else
                {System.out.println("You are getting out of the level");
                }
            }
            return(playerPos);
            }



    /**
     * @param args the command line arguments
     */
    public static void main(String[] args) {



        //δημιουργία του map
        int[] playerPos={15,8};
        int a= 1;
        String direction;
        int[][] map = Mapsystem2.map1;


            //εμφάνιση του map
            /**for (int i = 0; i < map.length; i++) {
            for (int j = 0; j < map[i].length; j++) {
                System.out.print(map[i][j]);
            }
            System.out.println("");
            }*/
        while (a==1)
        {

            System.out.println("Where do you want to go?");
            System.out.println("");

            Scanner in = new Scanner(System.in);  


            direction = in.nextLine();
            //System.out.println("You entered"+direction);
            playerPos=Mapsystem2.movement(direction, playerPos);

            //System.out.println(playerPos[0]);
            //System.out.println(playerPos[1]);


            //a= in.nextInt(); 
            //System.out.println("You entered integer "+a); 

        }
}

But i don't think this is an efficient way to do it and it's very confusing, i would love some ideas and help. Thank you.

\$\endgroup\$

closed as off-topic by esote, Eric Stein, IEatBagels, pacmaninbw, Mast May 22 at 12:08

This question appears to be off-topic. The users who voted to close gave this specific reason:

  • "Code not implemented or not working as intended: Code Review is a community where programmers peer-review your working code to address issues such as security, maintainability, performance, and scalability. We require that the code be working correctly, to the best of the author's knowledge, before proceeding with a review." – Eric Stein, IEatBagels, pacmaninbw
If this question can be reworded to fit the rules in the help center, please edit the question.

  • \$\begingroup\$ It's quite unclear why you're doing what you're doing. Does this work the way you want it to? Please take a look at the help center. \$\endgroup\$ – Mast May 22 at 12:08
2
\$\begingroup\$

Steve. Below are some thoughts on what you've provided. With respect, this code is really not at a state where it's reviewable per the guidelines in Code Review - it should be complete and working correctly. I understand why you're asking for a review, but maybe try to get the code further along?

Class names should be CamelCase, so MapSystem2 would be preferable.

Consistent indentation will make your code easier to read.

Commented-out code is a code smell.

Classes that aren't designed for extension should be declared final.

Variables that shouldn't be reassigned should be marked final.

Class variables should be private wherever possible. As designed, external classes can forcibly modify your maps without the knowledge of the Mapsystem2 class. That's probably not ideal.

Use meaningful variable names. d is not meaningful. direction is much better. Avoid unnecessary abbreviations, because you can't be sure readers of your code will translate them the same way you do.

Your maps would be easier to read if each internal array was placed on its own line.

Most of the comments don't serve any purpose, as they just describe what the code is doing. Comments are usually used to explain why the code is doing something, not what it is doing.

Please choose one language for your comments.

In Java we traditionally put { on the same line, not a newline by itself.

In Java we traditionally put whitespace between a control flow statement (if, while) and the opening paren, to visually differentiate them from method invocations.

In Java we traditionally put whitespace on both sides of operators and equals signs for readability.

You don't need () around a return value.

Tracking location using an int[] is not ideal. What happens when somebody passes in an array of size 1 or 3? Either use separate variables for x and y, use an existing library class, or create your own location class.

There is some argument on this point, but I find it much clearer to explicitly return when you know the return value, rather than storing a variable. I don't want to have to read the rest of a method just to figure out that the value doesn't change again before it gets returned.

In tileCheck, you assign x and y, then don't use them in the first if check.

The logic in tileCheck is very confusing. "If you try to walk off the map or enter a square with a 1, return 1. If you try to enter a square with a 0, tell the user they can't go there then return 0. Otherwise, return 0."

This class desperately needs documentation indicating what 0, 1, 2, 3, 4 actually mean. A reader will have no idea what they're dealing with.

Likewise, the meaning of the return value for tileCheck is unclear. It looks like 0 means an invalid move from later code, which would make the return value of the first check wrong?

Direction is best modeled with an enum. Using a map from input strings to directions would let you support multiple movement methods - "N", "North", "w" all move you north, etc.

There's a lot of repetition in the movement method. Everything that appears in all four branches should be pulled out of the conditionals.

It is counterintuitive that the north direction takes you towards the bottom of the map. I expect that's a bug.

You're calling tileCheck based on the user's current position before they move. That means that they can move off the map one square, and then get stuck and can't get back on it. Also a bug.

Scanner should be closed when you're done using it, and you shouldn't create a new scanner every time through your loop. Use a try-with-resources block to handle closing it.

Declare variables as closely as possible to where they're first used.

while (a == 1) is effectively while (true), since a is never changing.

map is unused in main.

\$\endgroup\$
1
\$\begingroup\$

From a quick review;

  • Comments should be in English
  • Commented code should be removed
  • a is not a great variable name, allowed would be better
  • If map[x][y] really can only contain 0 or 1, then you might as well just a = map[x][y]
  • The code is not consistently indented
  • You should call check=tileCheck only once before if ("north".equals(d))
  • You should code for else {System.out.println("You are getting out of the level");} only once before checking directions as well
  • Pure functionality, but you should either map the keyboard directions (8 is north, 2 is south etc.) or map 1 letter direction (n,w,e,w_ or even map the wasd keys.
\$\endgroup\$

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