4
\$\begingroup\$

This code feels long and clunky to me. How can I make it shorter, if possible?

I am writing for a libGDX game, and I need a skill algorithm. I want it so once the player picks up something (which I already have checked for) I set a, b or c to a certain SkillType, the enum down below. Then, I want to get the value of the specific SkillType, by using the enum's methods to get cooldown and cost.

public SkillType[] skills;
public SkillType a, b, c;
public int acost, bcost, ccost;
public int acooldown, bcooldown, ccooldown;

skills = new SkillType[3];
skills[0] = a; skills[1] = b; skills[2] = c;
acost = a.getCost(); bcost = b.getCost(); ccost = c.getCost();
acooldown = a.getCooldown(); bcooldown = b.getCooldown(); ccooldown = c.getCooldown();

public SkillType getSkill(int index) { return skills[index]; }
public void setSkill(int index, SkillType skill) { skills[index] = skill; }

This code declares the enum used above. It has all the skills that I want to use. It declares, inside the SkillType, its cost and cooldown. I do not want to change these values, and that's why I don't have setters.

public static enum SkillType {
        ROAR(1,3),
        DUST_CLOAK(2,10),
        THORN_CLOAK(2,10),
        SPARK(2,5),
        FLUTTER(0,0),
        EMBER(2,2),
        GLOW(0,0),
        SCRATCH(0,0),
        BITE(0,0);

        int cost;
        int cooldown;
        SkillType(int cost, int cooldown) {
                this.cost = cost;
                this.cooldown = cooldown;
        }
        public int getCooldown() { return cooldown; }
        public int getCost() { return cost; }
}

EDIT:

I have updated the enum class to include more items.

public class SkillData {

public SkillType[] skills;
public static final int MAX_SKILLS = 3;
public static final int SLOT_PRIMARY = 0;
public static final int SLOT_SECONDARY = 1;
public static final int SLOT_SECONDARY_TWO = 2;

public static enum SkillType {
    ROAR(1,3),
    DUST_CLOAK(2,10),
    THORN_CLOAK(2,10),
    SPARK(2,5),
    FLUTTER(0,0),
    EMBER(2,2),
    GLOW(0,0),
    SCRATCH(0,0),
    BITE(0,0);

    int cost;
    int cooldown;
    SkillType(int cost, int cooldown) {
        this.cost = cost;
        this.cooldown = cooldown;
    }
    public int getCooldown() { return cooldown; }
    public int getCost() { return cost; }
}

public SkillData() {
    skills = new SkillType[MAX_SKILLS];
}

public int getCost(int index) { return skills[index].getCost(); }
public int getCooldown(int index) { return skills[index].getCost(); }
public void setSkill(int index, SkillType skill) { skills[index] = skill; }
public SkillType getSkill(int index) { return skills[index]; }
public SkillType[] getSkills() { return skills; }

}

\$\endgroup\$
  • 1
    \$\begingroup\$ I've edited to separate the SkillType class from the first code block... can you kindly explain/edit the first code block to indicate if that's in another class, how they are used, etc.? Thanks! :) \$\endgroup\$ – h.j.k. Jun 24 '15 at 1:29
  • \$\begingroup\$ I am adding this whole patch of code to more existing code. The array skills[] is used to store a,b, and c so I can set the skill w/ an index. The ints are for storage of the variables so I can use them later, like when the player uses currentSkill, i can subtract a variable like energy from the cost. And then, i can set a timer so the player cannot use the skill until after the cooldown is done. \$\endgroup\$ – salarc Jun 24 '15 at 4:13
3
\$\begingroup\$

It would be much less cumbersome if you encapsulate the skill set into a separate class:

public class Skills {

    private static final int MAXSKILLS = 3;

    public final SkillType[] skills = new SkillType[MAXSKILLS];

    public int getTotalCost() {
        return skills[0].getCost() + skills[1].getCost() + skills[2].getCost();
    }

    public int getTotalCooldown() {
        return skills[0].getCooldown() + skills[1].getCooldown() + skills[2].getCooldown();
    }

    public SkillType getSkill(int index) {
        return skills[index];
    }

    public void setSkill(int index, SkillType skill) {
        skills[index] = skill;
    }
}

You can now do.

class Person {
    final Skills skills = new Skills();
    //...
}

and fold the required functionality into your Skills object.

\$\endgroup\$
  • \$\begingroup\$ The problem with that: I need to get each cooldown/cost individually. \$\endgroup\$ – salarc Jun 24 '15 at 18:31
  • \$\begingroup\$ @AndyMa - you still have getSkill. \$\endgroup\$ – OldCurmudgeon Jun 24 '15 at 18:41
4
\$\begingroup\$
skills = new SkillType[2];
skills[0] = a; skills[1] = b; skills[2] = c;

This is simply wrong. You make an array of size 2 and then put three things in it.

Also, you never set a, b, or c, so you might as well leave off the second line.

public int acost, bcost, ccost;
public int acooldown, bcooldown, ccooldown;

These are never read, so you could delete them.

In general, this code feels incomplete. Your enum looks about as short as it can be for the functionality. The rest of the code is unclear about what it is doing. You could probably delete almost all of it.

\$\endgroup\$
  • \$\begingroup\$ Ah, sorry about the array, i thought [2] counted as [3]. \$\endgroup\$ – salarc Jun 24 '15 at 4:07
  • \$\begingroup\$ The reason it feels incomplete is because im adding this to existing code. Alot of existing code. I want to create a system for a player. Once the player picks up a certain object, I need the different parameters. The reason a,b and c aren't set is because i want to set the later, using the setSkill(SkillType s) function. Once the player picks up the drop, I use the setSkill() function to set a, b or c. \$\endgroup\$ – salarc Jun 24 '15 at 4:09

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.