Question

I have a super class Creature that I have the following variables for in addition to others:

    public int terrain;
    public static final int DESERT = 0;
    public static final int MOUNTAIN = 1;
    public static final int FOREST = 2;

I would like to do something along the lines of this:

    public int getTerrain(){
        if(terrain = 0){
            terrain = DESERT;
        }
        if(terrain = 1){
            terrain = MOUNTAIN;
        }
        if(terrain = 2){
            terrain = FOREST;
        }
        return terrain;
    }

    public void setTerrain(int terrain){
        if(terrain = 0){
            this.terrain = DESERT;
        }
        if(terrain = 1){
            this.terrain = MOUNTAIN;
        }
        if(terrain = 2){
            this.terrain = FOREST;
        }

        this.terrain = terrain;
    }

This doesn't work but is there a way that I can do extra logic in my getters/setters? Or is this bad form? What is the best way to solve this problem?

Was it helpful?

Solution 2

Aside from the comparator issue (double-equals vs single-equals), there's a few other issues with your code that you can address that might clarify the role of getters and setters. You don't actually need to do any comparison in either of your methods.

If we declare that MOUNTAIN is always equal to 1 - then "1" and "MOUNTAIN" can be used interchangeably in our code. Let's replace all the digit literals (0, 1, 2) with the constant names instead.

public void setTerrain(int terrain){
    if(terrain == DESERT){
        this.terrain = DESERT;
    }
    if(terrain == MOUNTAIN){
        this.terrain = MOUNTAIN;
    }
    if(terrain == FOREST){
        this.terrain = FOREST;
    }
}

This is a bit redundant. Actually, if we instead replace terrain types with numbers, the issue might be more apparent:

public void setTerrain(int terrain){
    if(terrain == 0){
        this.terrain = 0;
    }
    if(terrain == 1){
        this.terrain = 1;
    }
    if(terrain == 2){
        this.terrain = 2;
    }
}

Imagine if you had 1000 terrain types - the code blows up even more.

public void setTerrain(int terrain){
    if(terrain == 0){
        this.terrain = 0;
    }
    if(terrain == 1){
        this.terrain = 1;
    }
    if(terrain == 2){
        this.terrain = 2;
    }
    if(terrain == 3){
        this.terrain = 3;
    }
    ...
    if(terrain == 999){
        this.terrain = 999;
    }
    if(terrain == 1000){
        this.terrain = 1000;
    }
}

Instead, you can just use something like this:

public void setTerrain(int terrain){
    this.terrain = terrain;
}

As Dino's answer suggests, it is always a great idea to add Input Validation to handle cases where someone sends a number that isn't one of your supported terrains:

public void setTerrain(int terrain){
    if (terrain < 0 || terrain > 2) {
        System.out.println("Invalid terrain number!");
    } else {
        this.terrain = terrain;
    }
}

Similarly, you can simplify your getter by just directly returning your Creature's terrain type - no comparisons needed!

public int getTerrain(){
    return this.terrain;
}

OTHER TIPS

The reason this doesn't work is because you do this

if(terrain = 1)

instead of this

if(terrain == 1)

Assignment uses =, comparison uses ==.

That being said: I'd go for an enum instead.

public enum Terrain {
    DESERT,
    MOUNTAIN,
    FOREST
}

Usage:

public class Main {
    private Terrain terrain;

    public static void main(String[] args) {
        Main obj = new Main();
        obj.setTerrain(Terrain.DESERT);
    }

    public Terrain getTerrain(){
        return terrain;
    }

    public void setTerrain(Terrain terrain){
        this.terrain = terrain;
    }
}

You can still use a numeric value as an alias as well.

In regards to what a getter/setter should contain: yes, you can perform logic in it. The entire idea behind using getters/setters is that you can provide additional logic (like validation or manipulation of the backing field) before returning the data.

You used the assignment operator =, so there is no comparison occurring. It looks like you're getting a compiler error for this code, because there's no boolean value for the if.

if(terrain = 0){

You meant to compare the values, so use the equality comparison operator:

if(terrain == 0){

and likewise for your other comparisons.

That being said, there is nothing wrong with "extra logic" in your setter methods. In fact, that is the preferred way of providing encapsulation and data integrity in your objects. You can go a step further and throw an IllegalArgumentException for unexpected int values, or you can define and use enums to prevent unexpected values.

Yes you can.

Apart from the "=", "==" mix up in your code (which is why it may not be working) you are setting the value in both setter and getter. Although nothing stops you from doing this, doing this isn't right. Also, you should consider initializing 'terrain' such as

public int terrain = DESERT; instead of

public int terrain;

This way your getter can be simplified to

public int getTerrain() { return terrain; }

The logic in your setter doesn't make much sense either. You may want to have something like this

public void setTerrain(int ter)
{
   //validate input. You could do a full invalid range check although 
   //I just checked here for negative values
   if (ter < 0) {
   //handle this use case. You want to validate input for public API of a class always.
   }
   //set value.
   terrain = ter;

   //Compare with the finals here and do whatever else needs to be done.
}

Update: Also consider

  1. Setting terrain' during construction. Too often work that should be done in the constructor is done in the setter. To avoid clutter, aid in re-use you can also use a validateTerrain( ) method for validating input both in the constructor and the setter. So the constructor will look something like

    // like someone mentioned enum is a good fit

    public Creature(int ter) { //setter can set the value same way if (validateTerrain(ter)) { terrain = ter; } }

  2. Getting rid of the setter (this needs #1) if 'terrain's value will not change over the life-time of the object containing it. This will make the property read-only after construction. Irrespective of this step, #1 should be done anyway IMO.

Nothing is stopping you from putting whatever you want in getters or setters. It's your code; you're in control of it.

However, those if statements are assignments, not comparisons. I think you're looking for ==, not =.

Licensed under: CC-BY-SA with attribution
Not affiliated with StackOverflow
scroll top