Problem
I’ve been coding for around a month, learning through CodeHS and coding within their sandbox tool. I am currently working on a text-based explorer/RPG game in order to practice Object-Oriented Programming in Java.
I wanted to know if the current relationship between the Location and the Explore class is optimal, as I am not sure if accessing the instance variables of Location within Explore is considered to be bad practice. Also, my current method of determining whether or not a generated location is for example a stream seems inefficient, as it simply checks if it contains the strings "stream" or "river". Any help with design is greatly appreciated as well.
The Explore Class:
public class Explore{
public Location location;
public Player player;
//distances from the town originated and to next town
public int distanceFrom;
public int distanceTo;
public Explore(Player player){
this.player = player;
this.distanceFrom = 0;
this.distanceTo = RNG.range(3, 6);
this.location = new Location(player);
}
public int getDistanceFrom(){
return distanceFrom;
}
public int getDistanceTo(){
return distanceTo;
}
public boolean isStream(){
return location.isStream;
}
public boolean canDrink(){
return location.canDrink;
}
public void drink(){
location.canDrink = false;
}
public String getDesc(){
return location.toString() + " " + distanceFrom + " km away from your origin";
}
public Location getLocation(){
return location;
}
public void advance(){
if(distanceFrom <= 5){
player.incrementDev(RNG.range(1, 4));
}else{
player.incrementDev(RNG.range(3, 6));
}
this.location = new Location(player);
this.distanceFrom++;
this.distanceTo--;
}
public boolean checkNearbyTown(){
if(distanceTo <= 1 && distanceTo > -1){
return true;
}else{
return false;
}
}
}
The Location Class:
public class Location
{
private Player player;
private String desc;
public boolean isStream = false;
public boolean isCave = false;
public boolean canDrink = false;
//add caves later
//public boolean canCave = false;
public Location(Player player){
this.player = player;
this.desc = RNG.locationDesc(player);
if((this.desc.indexOf("stream") != -1) || (this.desc.indexOf("river") != -1)){
this.isStream = true;
this.canDrink = true;
}
}
public String toString(){
return desc;
}
}
The RNG Class, which handles my games random events:
import java.util.*;
public class RNG{
//Handles all random events such as pulling a number from a range or generating a name/stats
public static int roll(int sides){
int aRoll = (int)(Math.random() * sides + 1);
return aRoll;
}
public static int range(double min, double max){
return (int)(Math.random() * (max - min + 1) + min);
}
public static String locationDesc(Player player){
int dev = player.getDev();
//takes in a player input in order to determine their insanity, making a more twister are
String[] desc;
if(dev <= 75){
desc = new String[]{"an unassuming wooded area", "an open flower field",
"a steady stream", "the entrance of a small cave", "a small river"};
}else{
desc = new String[]{"Spookier areas to add later"};
}
int index = (int)(Math.random() * desc.length);
return desc[index];
}
}
Solution
thanks for sharing your code!
I want to start by saying that I think this looks pretty good for someone only having been coding for around a month, but I think you’re getting some concepts mixed up or not quite sure how/where/why to use them.
The other comments and answers have already pointed out flaws with the Explore class, so I won’t repeat those
Also, my current method of determining whether or not a generated
location is for example a stream seems inefficient, as it simply
checks if it contains the strings “stream” or “river”
It’s a good sign that this jumped out as not being the best idea! There are a few problems with your design here, let’s have a look.
Your Location class contains these instance variables
public boolean isStream = false;
public boolean isCave = false;
public boolean canDrink = false;
There are 2 pretty big problems here
- You almost never want to make your instance variables public. You should use a getter if you actually need to see that variable outside the class (which would happen less often than you would think)
- Your location class contains information that specifies what type of location it is!
If we wanted to make a new Location, which I think is a very reasonable thing to do. We would need to go and change the code in the Location class itself. Ideally, if we wanted to make a brand new location, say a Town. All we should need to do is make a Town class, and slot it in. Let’s have a look at how we could do that.
Firstly, we could make the Location class, an interface instead
public interface Location {
void explore(Player player); // note explore is a method (verb), not a noun (class)
}
Now, in order to be a Location, we just need to implement an explore method. So instead of maintaining booleans which describe the Location concrete class, we can make many smaller implementations instead. Let’s have a look at a potential Cave implementation.
public class Cave implements Location {
@Override
public void explore(Player player) {
System.out.println(player.getName() + " is at the entrance of a small cave.");
// do scary cave things to the player here.
}
}
Now, the Location interface itself doesn’t know anything about whether or not it’s a Cave, or Stream or Town etc. Because it doesn’t need to know!
What about a Town?
public class Town implements Location {
@Override
public void explore(Player player) {
System.out.println(player.getName() + " arrived at a small town.");
// do whatever you want to do with the player in the town here.
}
}
again, very similar to Cave, you could even consider making an Abstract Base Class instead of an interface, but let’s keep going with this for now.
And one more example
public class River implements Location{
@Override
public void explore(Player player) {
System.out.println(player.getName() + " is at a river, it's nice and relaxing.");
}
}
Okay, so now we have more smaller classes that each are specialized, but how is this good? My main goal here, is to not need to alter code when I create a new implementation of Location
You don’t have an example of the Player class or the main Runner class, so for now a Player just has a getName method. You can of course do whatever you want in your Player class.
Instead of an Explore class, let’s make a Game class. That’s what this is after all, an RPG.
public class Game {
private List<Location> locations;
private Player player;
public Game(List<Location> locations, Player player) {
this.locations = new LinkedList<>(locations);
this.player = player;
}
public void play() {
System.out.println(player.getName() + " began their adventure.");
for (Location location : locations) {
location.explore(player);
System.out.println(player.getName() + " is moving on to the next location.");
}
}
}
So there’s not a whole lot of code here, our Game just makes runes a player through all the levels.
It we then just do a short Runner class to tie it all together.
public class Runner {
public static void main(String[] args) {
Player player = new Player("Bob The Explorer");
List<Location> locations = new LinkedList<>();
locations.add(new Cave());
locations.add(new Town());
Game rpg = new Game(locations, player);
rpg.play();
}
}
This short example gives the following.
Bob The Explorer began their adventure.
Bob The Explorer is at the entrance of a small cave.
Bob The Explorer is moving on to the next location.
Bob The Explorer arrived at a small town.
Bob The Explorer is moving on to the next location.
I’ve used a larger number of smaller classes here. We now have the benefit of easily being able to add a new Location to our game, we just make a new class, implement Location, and say what should happen when the explore method is called!
Of course if you needed to do more things than just you explore, you could add new methods. But I’m a big fan of having as few public methods as possible in any given class.
Your logic for checking number of tiles to towns etc. could all be done in the Game class.
In your example, you have some logic around maintaining knowledge about whether or not you can drink from a location, I’ll just add one more quick example about how you can do this, let’s rework the River class just a little bit.
public class River implements Location{
private boolean canDrink;
public River(){
canDrink = true;
}
@Override
public void explore(Player player) {
System.out.println(player.getName() + " is at a river, it's nice and relaxing.");
if(canDrink){
canDrink = false;
player.restoreStamina(); // or whatever "drinking" from the river should do!
System.out.println(player.getName() + " feels refreshed after drinking from the river.");
} else {
System.out.println(player.getName() + " has already drank from the river, it's all gone!");
}
}
}
It’s still a valid implementation of Location, but we can add more stateful behaviour to it. (And the Game class will never know!)
Let’s have a look at what main looks like now
public class Runner {
public static void main(String[] args) {
Player player = new Player("Bob The Explorer");
List<Location> locations = new LinkedList<>();
Location river = new River();
locations.add(new Cave());
locations.add(river);
locations.add(new Town());
locations.add(river);
Game rpg = new Game(locations, player);
rpg.play();
}
}
With an output of
Bob The Explorer began their adventure.
Bob The Explorer is at the entrance of a small cave.
Bob The Explorer is moving on to the next location.
Bob The Explorer is at a river, it's nice and relaxing.
Bob The Explorer feels refreshed after drinking from the river.
Bob The Explorer is moving on to the next location.
Bob The Explorer arrived at a small town.
Bob The Explorer is moving on to the next location.
Bob The Explorer is at a river, it's nice and relaxing.
Bob The Explorer has already drank from the river, it's all gone!
Bob The Explorer is moving on to the next location.
So we’ve added new functionality to an existing location (we could have just as easily added a new one!)
And the Game class remains unchanged.
I’ve done location.explore(player), this might make more sense as being player.explore(location) or location.beExploredBy(player)
Hopefully this review was helpful for you!
Explore is a verb, so your Explore class should be called Exploration, or probably something more descriptive like PlayerStatus. And you are defining state in Location and then just re-exposing it in Explore. Merge the two classes if you notice they are bleeding. You are right to be questioning that and noticing something isn’t right. What is the point of Explore? Merge it into Location.
Also the RNG is bound to become a god-class, if you allow methods like locationDesc there.
RNG‘s job should be to provide random numbers, or return a random element from a list, array, etc. The location description should come from Location.
Something like this:
class Location
...
public static final String [] descriptions= { .... }
public static final String [] spookyDescriptions= { .... }
...
public String getDescription()
{
//get appropriate descriptions for current player;
String[] desc = getDescriptionsForPlayer(this.player)
return RNG.pick( desc );
}
}