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;
    }

possible duplicate of [How do I compare strings in Java?](http://stackoverflow.com/questions/513832/how-do-i-compare-strings-in-java) — JB Nizet, Mar 17 '14 at 07:35
Offtopic: consider to change your leveling internals to integer, and just increment/decrement it. Only convert to string when you need to return a level to user in human-readable form (looking up in array or hash table). This will reduce hairy mess inside your class, probably could increase performance, and improve scalability (adding new levels is just adding an element to array). — Ivan Aksamentov - Drop, Mar 17 '14 at 10:32

score 2 · Answer 1 · edited May 23 '17 at 12:27

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.

score 0 · Accepted Answer · answered Mar 17 '14 at 11:05

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.

You address the string comparison issue, but you leave out returning `true` when no action is actually performed. The OP chose to declare their fields as `private`, I think it would be best to write getters and setters if they wanted to access such variables from outside the class. — Tonio, Mar 17 '14 at 12:16

score 0 · Answer 3 · answered Mar 17 '14 at 11:21

0

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 :)

answered Mar 17 '14 at 11:21

Gorbles

1,169
12
27

What's wrong with using an `enum`? The way I see it, you did exactly that but you keep the 'burden' of handling strings and using the more costly `equals` comparison. As @Drop posted in the comments, I think it's better to look up level-string correspondance for human interaction only. – Tonio Mar 17 '14 at 12:21
There's nothing wrong with a set of enums. I prefer using a list of Strings personally, as you only need to edit the list to add, change or remove ranks. Yours doesn't scale as well for a much larger list, as you'd have to define a switch statement every time. On the flipside, your supports more complicated promotions/demotions better :) – Gorbles Mar 17 '14 at 12:24
I see your point, but actually you're not forced to update the switch statement all the time. I did not include it for simplicity's sake, but there are ways to iterate over `enum` values. Maybe I'll complete my answer, but it felt to me a little bit off topic at the time. – Tonio Mar 17 '14 at 12:30
I didn't know about `enum.getValues` beforehand, which is one reason why I haven't used them. I also didn't know someone deleted my comment on your answer, as I said I upvoted your answer for using enums! – Gorbles Mar 17 '14 at 12:39
No problem ;) -- I added some 'getting started' information to my post, so that it's not too much off topic. – Tonio Mar 17 '14 at 13:07

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

3 Answers3