Take the 2-minute tour ×
Code Review Stack Exchange is a question and answer site for peer programmer code reviews. It's 100% free, no registration required.

I have four different classes. First being a base class for the second and third. The fourth being a container class that holds instances of second and third (stored as first).

My base class for PC and Companion

public class Controllable
{
    protected String name;
    protected int x;

    public Controllable (String name) {
        this.name = name;
    }

    public void move () {
        x++;
    }
}

public class PC extends Controllable
{
    private int someUniqueVariable;

    public PC (String name, int someUniqueVariable) {
        super (name);
        this.someUniqueVariable = someUniqueVariable;
    }
}

public class Companion extends Controllable
{
    public Companion (String name) {
        super (name);
    }
}

And finally the wrapper/container class.

public class Party
{
    private PC pc;
    private List<Controllable> members;

    public Party () {
        PC = new PC ("PC", 1); // edit #1: should be pc, bad copy-paste
        members = new ArrayList<Controllable>();
        members.add (PC);
        members.add (new Companion ("Companion1");
        members.add (new Companion ("Companion2");
    }

    public moveAll () {
        for (Controllable m : members) {
            m.move();
        }
    }

    public getPC () {
        return pc;
    }
}

Now if I want to move all Controllable I will iterate over members. If there are pc specifics I will simply call the PC and use its move function. PC will be more separated from Companion later on.

I would mostly like feedback on the design of my code.

Edit #1:

The most critical area were I want feedback is this. Most things are shared between Companion and PC, however there are some unique features of PC. In 99% I want to be able to iterate over all of the party members In more rare cases I want to only access the Companions and sometimes I only want to access the PC.

public Party () {
    pc = new PC ("PC", 1);
    members = new ArrayList<>();
    members.add (PC);
    members.add (new Companion ("Companion1");
    members.add (new Companion ("Companion2");
}

Minor edit to show an error in the code.

share|improve this question
    
Looks good, however, would it be possible to make an array of Companions a property of PC? That is only assuming that the Companions will always follow the PC. –  CBredlow Dec 2 '14 at 0:53
    
@CBredlow, Companions will always follow the PC, but not vice-verse. So possible yes, however doesn't that break the design a bit? Trying to move those parts to Party. –  Emz Dec 2 '14 at 1:11
    
I don't think it would, but it was just a thought. It may add more work so it might not be a good idea to do my suggestion. –  CBredlow Dec 2 '14 at 1:22

1 Answer 1

Controllable class

  • can be abstract, it doesn't seem you will instantiate it
  • int x field shall be private, you don't use it anywhere

Party class

  • you declared variable of type PC with lower letters so object assignment shall be too pc = new PC ("PC", 1);
  • members = new ArrayList<Controllable>(); with Java 8 it is not necessary to declare list's type if you do this with variable declaration, so members = new ArrayList<>(); is enough

It is a good practice to make method arguments final, as well class fields if they are provided through constructor. It means their declared type cannot be changed.

share|improve this answer
    
Hi, and welcome to Code Review. Good spotting for the PC -> pc bug. That's what happens when people try to 'edit' or 'fix' their code before posting to Code Review.... it leads to unexpected bugs. The Diamond operator <> is available in Java7 too, FYI. –  rolfl Dec 4 '14 at 23:12
    
The ´abstract´ part makes sense. The int x is just a temporary variable. The PC = new PC ("PC", 1); is a typo. Basically everything is temporary in this code apart from the storing. I should emphasize that a bit more I guess. –  Emz Dec 4 '14 at 23:26
    
@Leszek, I have never ever seen final in method arguments. Do you have perhaps have some good examples of where I can read more about that? –  Emz Dec 8 '14 at 21:40
    

Your Answer

 
discard

By posting your answer, you agree to the privacy policy and terms of service.

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