String-related issue in Java: why isn't my level promotion/downgrade updated correctly?

Question

I am trying to write this program in java.

I can read the string from the constructor and separate the string by using delimiters. However, every time, I try to call methods such as promote() for those values, the value just remains the same. The boolean method promote() displays true, but the level just doesn't increases.

For example: if the string from the constructor is : "Harry#wizard#broom", then:

name <- "Harry"
level <- "wizard"
supplies <- "broom"

name, level and broom are all the private data instances (I don't know if I should be using data instances or local variables).

However, whenever I call harry.promote() it returns true but the level just stays the same, it doesn't get promoted. The initial value in level seems to overwrite it.

I have written the following code:

import java.util.Scanner;
import java.io.*;

public class Magician
{
    private String name;
    private String level;
    private String supplies;
    private double health;
    private int galleons;

    public Magician(String phrase) //phrase will be sth like this: Harry#wizard#broom#staff
    {

        health = 4000;
        galleons = 200;
        Scanner firstScan = new Scanner(details);
        firstScan.useDelimiter("#");
        name = firstScan.next();
        level = firstScan.next();
        if (firstScan.hasNext())
        {
            supplies = firstScan.next(); 
            while (firstScan.hasNext())
            {        
                supplies = supplies + " " + firstScan.next();
            }
        }
        else 
        {
            supplies ="";
        } 

    }
    public String getName()
    {
        return name;
    }
    public String getLevel()
    {
        return level;
    }
    public int getGalleons()
    {
        return galleons;
    }
    public double getHealth()
    {
        return health;
    }
    public String getSupplies()
    {
        return supplies;
    }
    //Mutators 
    public void setName(String nameUpdate)
    {
        name = nameUpdate;
    }
    public void setLevel(String levelUpdate)
    {
        level =levelUpdate;
    }
    public void setGalleons(int galleonsUpdate)
    {
        galleons = galleonsUpdate;
    }
    public void setHealth(double healthUpdate)
    {
        health = healthUpdate;
    }
    public void setSupplies(String suppliesUpdate)
    {
        supplies = suppliesUpdate;
    }
    // boolean promote, promotes a level up
    public boolean promote()
    {
        if (level == "apprentice")
        {
            level = "wizard";
            galleons +=100;
            return true;
        }
        else if (level == "wizard")
        {
            level = "mage";
            galleons +=100;
            return true;
        }
        else if (level == "mage")
        {
            level = "sorcerer";
            galleons +=100;
            return true;
        }
        else if (level == "sorcerer")
        {
            level = "shaman";
            galleons +=100;
            return true;
        }
        else if (level == "shaman")
        {
            return false;
        }
        return true;
    }
    public boolean downgradeLevel()
    {
        if (level == "shaman")
        {
            level = "sorcerer";
            return true;
        }
        else if (level == "sorcerer")
        {
            level = "mage";
            return true;
        }
        else if (level == "mage")
        {
            level = "wizard";
            return true;
        }
        else if (level == "wizard")
        {
            level = "apprentice";
            return true;
        }
        else if (level == "apprentice")
        {
            return false;
        }
        if(galleons>= 100)
        {
            galleons -=100;
        }
        else
        {
            galleons =0;
        }
        return true;
    }        

Answer 1

Try this:-

// boolean promote, promotes a level up
    public boolean promote()
    {
        if (level.equals("apprentice"))
        {
            level = "wizard";
            galleons +=100;
            return true;
        }
        else if (level.equals("wizard"))
        {
            level = "mage";
            galleons +=100;
            return true;
        }
        else if (level.equals("mage"))
        {
            level = "sorcerer";
            galleons +=100;
            return true;
        }
        else if (level.equals("sorcerer"))
        {
            level = "shaman";
            galleons +=100;
            return true;
        }
        else if (level.equals("shaman"))
        {
            return false;
        }
        return true;
    }

If you need to access the variable from main() then you need to declare the variable as static.

public static void main(String str[]){
        Magician magg=new Magician("Harry#wizard#broom#staff");

        System.out.println("Level before promote is ::"+level);
        magg.promote();
        System.out.println("Level after promote is::"+level);
    }

Output :-

Level before promote is ::wizard
Level after promote is::mage

Its working now.

Hope it will help you.

Answer 2

I would replace:

    else if (level == "shaman")
    {
        return false;
    }
    return true;

by:

    else if (level.equals("shaman"))
    {
        return false;
    }

    System.err.println("Invalid/corrupted level name! ['" + level + "']");
    return false;

Indeed, if I can rephrase your algorithm:

if level is upgradable then
    promote it
    return true

if level is top level
    don't do anything
    return false

// we shouldn't be in this place!
// A level is either upgradable or it is the top level...
// if we are, there is some mistake in here
warn user
return false

and I would do the same for downgrade().

Whether or not you add the error message, you shouldn't return true for level promotion/downgrade if you don't actually perform any promotion/downgrade operation.

It usually is a good practice to check for unexpected behavior and raise an exception (or at least write to the error output) when appropriate. In this case, it might have alerted you at run time that you never entered any of your tests and you would probably have seen that the problem was somehow related to the equality check for the strings.

As @Rafa El and @JB Nizet have stated in the comments, you shouldn't use == to compare strings in Java. For reference, I'm adding the link @JB Nizet provided in the comments: How do I compare strings in Java?

Note: Why don't you use an enum to store the possible levels? that way you know there cannot be any mistake in the string that is passed to your level object.

public enum Level {
    APPRENTICE,
    WIZARD,
    MAGE,
    SORCERER,
    SHAMAN,
}

and then:

Level level = Level.APPRENTICE;

switch (level) {
    case APPRENTICE:
        level = Level.WIZARD;
        galleons += 100;
        return true;
    case ...:
        ...
}

Also, because upgrade/downgrade is very much only a value shift in the enum, you could write a more generic method for that shift, and it would thus be easier to add/delete a level or change their orders as your project evolves, without needing to edit all your test cases.

As this point is slightly off topic, I'll keep it brief. Feel free to ask for precision if you have trouble with it. Basically, you can use:

• enum.toString() ("returns the name of this enum constant, as contained in the declaration"),
• iterate over enum.values() to iterate over an enum values,
• enum.ordinal() ("returns the ordinal of this enumeration constant (its position in its enum declaration, where the initial constant is assigned an ordinal of zero).")

Aside from the enum documentation, there are plenty of examples to help you get started.

Answer 3

To improve on JDeveloper's answer, you could include a list that contains all possible promotion values:

private String[] ranks = new String[] {
                    "Apprentice",
                    "Wizard",
                    "Mage",
                    "Sorceror",
                    "Shaman"};

public boolean promote(String existingRank)  {
    // for each rank that exists
    for(int n = 0; n < ranks.size(); n++) {
        // if we find our rank, the next one should be it's promotion
        if(existingRank.equals(ranks[n])) {
            level = ranks[n + 1];
            galleons += 100;
            return true;
        }
    }

    // if we fail to find a promotion, it's failed
    return false;
}

public boolean demote(String existingRank) {
    for(int n = 0; n < ranks.size(); n++) {
        if(existingRank.equals(ranks[n])) {
            level = ranks[n - 1];
            galleons += 100;
            return true;
        }
    }

    return false;
}

You could even collapse this into a single method (boolean demote then controls the level = line).

Also consider checking for the length of the array so you don't get an OutOfBounds exception when promoting a Shaman or demoting an Apprentice :)