72

As a fairly green Java coder I've set myself the hefty challenge of trying to write a simple text adventure. Unsurprisingly, I've encountered difficulties already!

I'm trying to give my Location class a property to store which exits it contains. I've used a boolean array for this, to essentially hold true/false values representing each exit. I'm not entirely convinced that

a) this is the most efficient way to do this and

b) that I'm using the right code to populate the array.

I would appreciate any and all feedback, even if it is for a complete code over-haul!

At present, when instantiating a Location I generate a String which I send through to the setExits method:

    String e = "N S U";
    secretRoom.setExits(e);

In the Location class, setExits looks like this:

public void setExits(String e) {
    if (e.contains("N"))
        bexits[0] = true;
    else if (e.contains("W"))
        bexits[1] = true;
    else if (e.contains("S"))
        bexits[2] = true;
    else if (e.contains("E"))
        bexits[3] = true;
    else if (e.contains("U"))
        bexits[4] = true;
    else if (e.contains("D"))
        bexits[5] = true;
}

I'll be honest, I think this looks particularly clunky, but I couldn't think of another way to do it. I'm also not entirely sure now how to write the getExits method...

Any help would be welcome!

NoobEditor
  • 15,563
  • 19
  • 81
  • 112
lordchancellor
  • 3,847
  • 4
  • 26
  • 26
  • 36
    Man, if only all "green" programmers could ask questions this way. May I also suggest [codereview](http://codereview.stackexchange.com) for questions like these that only ask for improvement? Good luck. – MarioDS Jul 09 '14 at 10:46
  • 1
    @MDeSchaepmeester : bad in English...what does *green Java coder* means?? :o – NoobEditor Jul 09 '14 at 10:48
  • 1
    @NoobEditor - It means 'new'. – Rudi Kershaw Jul 09 '14 at 10:49
  • @Rudi : hmmmm....why not just use *new* then rather than *green Java coder*, makes english difficult to understand!! :D – NoobEditor Jul 09 '14 at 10:51
  • 1
    Green is a pretty common English term http://www.wisegeek.com/what-does-it-mean-when-a-person-is-green.htm#didyouknowout – Ross Drew Jul 09 '14 at 10:58
  • 3
    Have you chosen this project specifically to learn Java better, or because text adventures are awesome? For the former, good going, you'll learn a lot! But if it's the latter, may I recommend Inform 7 or TADS? – Paul Z Jul 09 '14 at 18:06
  • 1
    Coincidentally, I learned Java from [this book](http://www.bluej.org/objects-first/), which has, among a bunch of projects, a text-adventure one. Highly recommended, in general. – Sherz Jul 10 '14 at 04:52
  • 2
    Thanks for the compliments and advice guys! @PaulZ, I've decided to do a text adventure to get better at Java (as well as because they are awesome!) I've done an online course that wasn't so great, so I thought I would just take the plunge and self-teach the rest. – lordchancellor Jul 10 '14 at 10:52
  • Also, @Sherz, many thanks for the book recommendation. I shall be sure to check it out! – lordchancellor Jul 10 '14 at 10:54

14 Answers14

128

The most efficient and expressive way is the following:

Use enums as Exits and use an EnumSet to store them. EnumSet is an efficient Set implementation that uses a bit field to represent the enum constants.

Here is how you can do it:

public enum Exit { North, West, South, East, Up, Down; }

EnumSet<Exit> set = EnumSet.noneOf(Exit.class); // An empty set.

// Now you can simply add or remove exits, everything will be stored compactly

set.add(Exit.North); // Add exit
set.contains(Exit.West); // Test if an exit is present
set.remove(Exit.South); //Remove an exit

Enum set will store all exits in a single long internally, so your code is expressive, fast, and saves a lot of memory.

gexicide
  • 38,535
  • 21
  • 92
  • 152
  • 3
    I love the simplicity of this answer! To make the code slightly more readable, I would also make each enum direction the full name, rather than the first letter. I.e `public enum Exit { North, West, South, East, Up, Down; }` – Dylan Watson Jul 09 '14 at 14:40
  • 1
    Very elegant solution. I've never seen `EnumSet` used like this before, but it looks like a great idiom to get used to. – ApproachingDarknessFish Jul 09 '14 at 21:47
  • It's the nicest solution, but not the most efficient. I don't believe the JVM can eliminate all the virtual call overhead, but I'd hope it's pretty fast. The most efficient way would be doing all the boring stuff manually (with bitwise operations and `int`-typed constants, terrible). – maaartinus Jul 09 '14 at 22:16
  • They key here is "Enum set will store all exits in a single long internally, so your code is expressive, fast, and saves a lot of memory." The only downside I can see is if the OP's "array" would be longer than 64 bits (assuming 64-bit arch). I'm not a Java person, but how would `EnumSet` work with more than `long`'s max? – zamnuts Jul 09 '14 at 23:41
  • 2
    @zamnuts The implementation of EnumSet uses JumboEnumSet if there are too many elements in the Enum. See http://grepcode.com/file/repository.grepcode.com/java/root/jdk/openjdk/6-b14/java/util/JumboEnumSet.java#JumboEnumSet – WW. Jul 10 '14 at 05:51
  • @zamnuts: WW is right, Java will switch to another implementation for larger enums. However, I think 64 bit should be sufficient for the directions used here. – gexicide Jul 10 '14 at 08:21
  • 1
    @maaartinus: I am not sure about that! I guess because the enum set of type `Exit` will always be the same class (RegularEnumSet), the JIT might be very well able to eliminate all virtual calls and inline everything. Using `int` in order to get rid of some virtual calls that are very likely to be optimized out smells a lot like premature optimization. – gexicide Jul 10 '14 at 08:22
  • 1
    @gexicide Agreed, it might be possible. A benchmark would tell us if someone was curious enough. Even when not, I'd think at least twice before using anything but `EnumSet` here. – maaartinus Jul 10 '14 at 08:32
  • 1
    As OP might want to use the String somewhere else (as he says, text adventures), you might want to include the possibility to add final fields to an enum. – WorldSEnder Jul 14 '14 at 02:14
29

Is there any reason why you are doing this with Strings and aren't passing in booleans, i.e.

public void setExits(boolean N, boolean E, boolean S, boolean W, boolean U, boolean D) 

Or having setters?

public void setNorthOpen(boolean open)
{
  bexits[4] = open;
}

Secondly, why are you storing the exits as an array of booleans, it's a small finite set, why not just

boolean N,S,E,W,U,D;

As then you don't need to keep track of which number in the array each direction is.

Also

This is a correct answer (if not completely optimal like that of @gexicide) but I fully encourage anyone to look at the other answers here for an interesting look at how things can be done in Java in different ways.

For future reference

Code which works belongs on Code Review, not Stack Overflow. Although as @kajacx pointed out, this code shouldn't -in fact- work.

Community
  • 1
  • 1
Ross Drew
  • 8,163
  • 2
  • 41
  • 53
  • 1
    Thanks @Ross, I like the look of the setters you have suggested. As I mentioned, I am very new to this so I am kind of blundering my way through! Thank you though, I appreciate your help. – lordchancellor Jul 09 '14 at 10:52
  • 2
    Also, thanks for the point about Code Review - I didn't know this but shall remember for future reference. – lordchancellor Jul 09 '14 at 10:52
  • Some of the answers on here are really good if you want to stick with the `String`s but unless you have a special reason to do so, I'd suggest moving to all `booleans` like in my example. – Ross Drew Jul 09 '14 at 10:53
  • 14
    setExits(false, true, false, true) will be horrible to read and the enum variant suggested in another answer is just superior. – OliverS Jul 09 '14 at 13:07
  • 1
    I agree, multiple aguments aren't great, that's why I suggested the accessor methods. I also agree that ideally the `EnumSet` is the nicest option but I had read that the OP was "green" and thought something simple as a stepping stone away from the obviously painful `String` passing was a better option. I would normally have expanded my answer to be more advanced but @gexicide answered in the meantime and I don't want to answer steal and both are now here for anyone who wants it. Down voting because its not the absolute optimal answer isn't really right though eh? – Ross Drew Jul 10 '14 at 08:27
15

OK, first of all, your setExits() method will not work as intended, chained if-elseif will maximally execute 1 branch of code, for example:

if (e.contains("N"))
    bexits[0] = true;
else if (e.contains("W"))
    bexits[1] = true;

Even if e contains both N and W, only bexits[0] will be set. Also this method will only add exits (for example calling setExits("") will not delete any existing exits.

I would change that method to:

bexits[0] = e.contains("N");
bexits[1] = e.contains("W");
...

Also, i definetly wouldn't remember that north is on index 0, west in on 1, ... so a common practice is to name your indexes using final static constants:

public static final int NORTH = 0;
public static final int WEST = 1;
...

Then you can write in your setExits method:

bexits[NORTH] = e.contains("N");
bexits[WEST] = e.contains("W");
...

(much more readible)

Finally, if you want your code even more well-arranged, you can make a Exits class representing avaliable exits, and backed by boolean array. Then on place where you create your String, you could create this class instead and save yourself work with generating and then parsing a string.

EDIT:

as @gexicide answers, there is a really handy class EnumSet which would be probably better for representing the exits than bollean array.

kajacx
  • 12,361
  • 5
  • 43
  • 70
  • 8
    `public static final int` feels like Java 1.4, and was replaced with enums instead – EpicPandaForce Jul 09 '14 at 10:59
  • You will use that as index to accsess an array, i like it better than using enum and grabbing enum's id or something to get the index. – kajacx Jul 09 '14 at 11:02
  • That's exactly what my answer used below. Well, it's true that you'd need to use `Directions.NORTH.ordinal()` for the index of a specific direction... – EpicPandaForce Jul 09 '14 at 11:04
  • On the other hand, the `EnumSet` looks really cool, didn't know that class, i'll add it to the answer. – kajacx Jul 09 '14 at 11:05
9

The EnumSet in the other answer is the best way to do this, I just wanted to add one more thing though for the future when you start looking not just at whether you can move but where you are moving to.

As well as EnumSet you also have EnumMap.

If you define a Room class/interface then inside the Room class you can have

Map<Direction, Room> exits = new EnumMap<>(Direction.class);

You can now add your links into the map as follows:

exits.put(Direction.NORTH, theRoomNorthOfMe);

Then your code to move between rooms can be very general purpose:

Room destination=currentRoom.getExit(directionMoved);

if (destination == null) {
    // Cannot move that way
} else {
    // Handle move to destination
}
Tim B
  • 40,716
  • 16
  • 83
  • 128
7

I would create an Exit enum and on the location class just set a list of Exit objects.

so it would be something like:

public enum Exit { N, S, E, W, U, D }

List<Exit> exits = parseExits(String exitString);
location.setExits(exits);
Ross Drew
  • 8,163
  • 2
  • 41
  • 53
niekname
  • 2,528
  • 1
  • 13
  • 27
6

Given what your code looks like, this is the most readable implementation I could come up with:

public class Exits {
    private static final char[] DIRECTIONS = "NSEWUD".toCharArray();

    public static void main(String... args) {
        String input = "N S E";
        boolean[] exits = new boolean[DIRECTIONS.length];

        for(int i = 0; i< exits.length; i++) {
            if (input.indexOf(DIRECTIONS[i]) >= 0) {
                exits[i] = true;
            }
        }
    }
}

That being said, there's a number of cleaner solutions possible. Personally I would go with enums and an EnumSet.

By the way, your original code is incorrect, as it will set as most one value in the array to true.

Robby Cornelissen
  • 91,784
  • 22
  • 134
  • 156
3

If you're defining exits as a string, you should use it. I would do it like:

public class LocationWithExits {
    public static final String NORTH_EXIT="[N]";
    public static final String SOUTH_EXIT="[S]";
    public static final String EAST_EXIT="[E]";
    public static final String WEST_EXIT="[W]";

    private final String exitLocations;

    public LocationWithExits(String exitLocations) {
        this.exitLocations = exitLocations;
    }
    public boolean hasNorthExit(){
        return exitLocations.contains(NORTH_EXIT);
    }
    public static void main(String[] args) {
        LocationWithExits testLocation=new LocationWithExits(NORTH_EXIT+SOUTH_EXIT);
        System.out.println("Has exit on north?: "+testLocation.hasNorthExit());
    }


}

using array of booleans might cause a lot of problems if you forget what exactly means bexits[0]. Os it for north or south? etc.

or you can just use enums and list of exits available . Then in methid test if list contain a certain enum value

T.G
  • 1,913
  • 1
  • 16
  • 29
3

Personally, I think you can hack it around a bit using an enum and turn the following:

public void setExits(String e) {
    if (e.contains("N"))
        bexits[0] = true;
    else if (e.contains("W"))
        bexits[1] = true;
    else if (e.contains("S"))
        bexits[2] = true;
    else if (e.contains("E"))
        bexits[3] = true;
    else if (e.contains("U"))
        bexits[4] = true;
    else if (e.contains("D"))
        bexits[5] = true;
}

into

public enum Directions
{
    NORTH("N"),
    WEST("W"),
    SOUTH("S"),
    EAST("E"),
    UP("U"),
    DOWN("D");

    private String identifier;

    private Directions(String identifier)
    {
        this.identifier = identifier;
    }

    public String getIdentifier()
    {
        return identifier;
    }
}

and then do:

public void setExits(String e) 
{
  String[] exits = e.split(" ");
  for(String exit : exits)
  {
      for(Directions direction : Directions.values())
      {
          if(direction.getIdentifier().equals(exit))
          {
              bexits[direction.ordinal()] = true;
              break;
          }
      }
  }
}

Although after having written it down, I can't really tell you if it's that much better. It's easier to add new directions, that's for sure.

EpicPandaForce
  • 79,669
  • 27
  • 256
  • 428
  • I assumed that bexits will be created as `boolean bexits = new boolean[Directions.values().length];` – EpicPandaForce Jul 09 '14 at 10:55
  • To be honest, when I wrote my own code, what I did was use a `Map map = new HashMap<>();`, and use them as either `map.put(Directions.NORTH, false);` or `map.get(Directions.NORTH);`. It's slightly prettier than the one up above, but it's still not as pretty as the `EnumSet` by `gexicide` above. – EpicPandaForce Jul 09 '14 at 19:34
3

All the approaches listed in the answeres are good. But I think the approach you need to take depends on the way you are going to use the exit field. For example if you are going to handle exit as strings then Ross Drews approach would require a lot of if-else conditions and variables.

String exit = "N E";
String[] exits = exit.split(" ");
boolean N = false, E = false, S = false, W = false, U = false, D = false;
for(String e : exits){
    if(e.equalsIgnoreCase("N")){
        N = true;
    } else if(e.equalsIgnoreCase("E")){
        E = true;
    } else if(e.equalsIgnoreCase("W")){
        W= true;
    } else if(e.equalsIgnoreCase("U")){
        U = true;
    } else if(e.equalsIgnoreCase("D")){
        D = true;
    } else if(e.equalsIgnoreCase("S")){
        S = true;
    }
}
setExits(N, E, S, W, U, D);

Also if you have an exit and you want to check whether a location has that particular exit then again you will have to do the same

public boolean hasExit(String exit){
    if(e.equalsIgnoreCase("N")){
        return this.N; // Or the corresponding getter method
    } else if(e.equalsIgnoreCase("E")){
        return this.E;
    } else if(e.equalsIgnoreCase("W")){
        return this.W;
    } else if(e.equalsIgnoreCase("U")){
        return this.U;
    } else if(e.equalsIgnoreCase("D")){
        return this.D;
    } else if(e.equalsIgnoreCase("S")){
        return this.S;
    }
}

So if you are going to manipulate it as a string, in my opinion the best approach would be to go for list and enum. By this way you could do methods like hasExit, hasAnyExit, hasAllExits, hasNorthExit, hasSouthExit, getAvailableExits etc etc.. very easily. And considering the number of exits (6) using a list (or set) wont be an overhead. For example

Enum

public enum EXIT {
        EAST("E"),
        WEST("W"),
        NORTH("N"),
        SOUTH("S"),
        UP("U"),
        DOWN("D");

        private String exitCode;

        private EXIT(String exitCode) {
        this.exitCode = exitCode;
        }

        public String getExitCode() {
        return exitCode;
        }

        public static EXIT fromValue(String exitCode) {
            for (EXIT exit : values()) {
                if (exit.exitCode.equalsIgnoreCase(exitCode)) {
                    return exit;
                }
            }
            return null;
        }

        public static EXIT fromValue(char exitCode) {
            for (EXIT exit : values()) {
                if (exit.exitCode.equalsIgnoreCase(String.valueOf(exitCode))) {
                    return exit;
                }
            }
            return null;
        }
}

Location.java

import java.util.ArrayList;
import java.util.List;


public class Location {

    private List<EXIT> exits;

    public Location(){
        exits = new ArrayList<EXIT>();
    }

    public void setExits(String exits) {
        for(char exitCode :  exits.toCharArray()){
            EXIT exit = EXIT.fromValue(exitCode);
            if(exit != null){
                this.exits.add(exit);
            }
        }
    }

    public boolean hasExit(String exitCode){
        return exits.contains(EXIT.fromValue(exitCode));
    }

    public boolean hasAnyExit(String exits){
        for(char exitCode :  exits.toCharArray()){
            if(this.exits.contains(EXIT.fromValue(exitCode))){
                return true;
            }
        }
        return false;
    }

    public boolean hasAllExit(String exits){
        for(char exitCode :  exits.toCharArray()){
            EXIT exit = EXIT.fromValue(exitCode);
            if(exit != null && !this.exits.contains(exit)){
                return false;
            }
        }
        return true;
    }

    public boolean hasExit(char exitCode){
        return exits.contains(EXIT.fromValue(exitCode));
    }

    public boolean hasNorthExit(){
        return exits.contains(EXIT.NORTH);
    }

    public boolean hasSouthExit(){
        return exits.contains(EXIT.SOUTH);
    }

    public List<EXIT> getExits() {
        return exits;
    }

    public static void main(String args[]) {
        String exits = "N E W";
        Location location = new Location();
        location.setExits(exits);
        System.out.println(location.getExits());
        System.out.println(location.hasExit('W'));
        System.out.println(location.hasAllExit("N W"));
        System.out.println(location.hasAnyExit("U D"));
        System.out.println(location.hasNorthExit());
    }
}
Syam S
  • 8,421
  • 1
  • 26
  • 36
2

Why not this if you want a shorter code:

String symbols = "NWSEUD";
public void setExits(String e) {
  for (int i = 0; i < 6; i++) {
    bexits[i] = e.contains(symbols.charAt(i));
  }
}
Bogdan Alexandru
  • 5,394
  • 6
  • 34
  • 54
1

If you want a generic solution you can use a map, which maps from a key (in your case W, S, E.. ) to a corresponding value (in your case a boolean).

When you do a set, you update the value the key is associated with. When you do a get, you can take an argument key and simply retrieve the value of the key. This functionality does already exist in map, called put and get.

NoobEditor
  • 15,563
  • 19
  • 81
  • 112
Pphoenix
  • 1,423
  • 1
  • 15
  • 37
  • 3
    A Map to check for something being set or something existing is logically the same as having a Set and using the `contains` method. If the object is in the set, then the corresponding map would have had TRUE, is the object isn't in the set, then the corresponding map would have had FALSE. Not saying your answer is wrong, just another way to think of it that uses a little less space :) – Dan Temple Jul 09 '14 at 10:57
0

I really like the idea of assigning the exits from a String, because it makes for brief and readable code. Once that's done, I don't see why you would want to create a boolean array. If you have a String, just use it, although you might want to add some validation to prevent accidental assignment of strings containing unwanted characters:

private String exits;

public void setExits(String e) {
    if (!e.matches("[NSEWUD ]*")) throw new IllegalArgumentException();
    exits = e;
}

The only other thing I would add is a method canExit that you can call with a direction parameter; e.g., if (location.canExit('N')) ...:

public boolean canExit(char direction) {
    return exits.indexOf(direction) >= 0;
}

I like enums, but using them here seems like over-engineering to me, which will rapidly become annoying.


**Edit**: Actually, don't do this. It answers the wrong question, and it does something which doesn't need to be done. I just noticed @TimB's answer of using a map (an EnumMap) to associate directions with rooms. It makes sense.

I still feel that if you only need to track exit existence, a String is simple and effective, and anything else is over-complicating it. However, only knowing which exits are available isn't useful. You will want to go through those exits, and unless your game has a very plain layout it won't be doable for the code to infer the correct room for each direction, so you'll need to explicitly associate each direction with another room. So there seems to be no actual use for any method "setExits" which accepts a list of directions (regardless of how it's implemented internally).

Boann
  • 48,794
  • 16
  • 117
  • 146
0
public void setExits(String e)
{
    String directions="NwSEUD";
    for(int i=0;i<directions.length();i++)
    {
        if(e.contains(""+directions.charAt(i)))
        {
            bexits[i]=true;
            break;
        }
    }
}

the iterative way of doing the same thing..

Nitin9791
  • 1,124
  • 1
  • 14
  • 17
0

Long chains of else if statements should be replaced with switch statements.

Enums are the most expressive way to store such values as long as the efficiency is not a concern. Keep in mind that enum is a class, so creation of a new enum is associated with corresponding overhead.

sixtytrees
  • 1,156
  • 1
  • 10
  • 25