5
\$\begingroup\$

I was writing class Player with private fields and getters and setters. But the number of methods started to grow, I used lists and maps so additionally I had to create methods to get size of them and every item by index, key and etc. I got tired and decided to use public fields instead of getters and setters. Additionally, I grouped them into inner classes.

In the end, my class looks like this:

package com.game.quest.core.domain;

import com.game.quest.core.domain.armor.Accessory;
import com.game.quest.core.domain.armor.Armor;
import com.game.quest.core.domain.armor.Weapon;

import java.util.ArrayList;
import java.util.HashMap;
import java.util.List;
import java.util.Map;
import java.util.Map.Entry;

public class Player {

    public PlayerData is;
    public Chars crs;

    {
        is = new PlayerData();
        crs = new Chars();
    }

    public class PlayerData {

        public List<Stat> stats = new ArrayList<>();
        public List<Skill> skills = new ArrayList<>();
        public List<Title> titles = new ArrayList<>();
        public List<Object> inventory = new ArrayList<>();
        public Map<String, Quest> quests = new HashMap<>();
        public Map<String, Armor> armor = new HashMap<>();
        public Map<String, Weapon> weapons = new HashMap<>();
        public Map<String, Accessory> accessories = new HashMap<>();

        class PlayerDataHelper {

            public Map<String, Quest> filterQuestStatus(String filter) {
                Map<String, Quest> filtered = new HashMap<>();
                for (Entry<String, Quest> e : quests.entrySet()) {
                    if (e.getValue().getStatus().equals(filter)) {
                        filtered.put(e.getKey(), e.getValue());
                    }
                }
                return filtered;
            }

            public List<Armor> getArmor() {
                return new ArrayList<>(armor.values());
            }

            public void equipArmor(String bodyPart, Armor armor) {
                unequipArmor(bodyPart);
                PlayerData.this.armor.put(bodyPart, armor);
            }

            public void unequipArmor(String bodyPart) {
                Armor a = armor.get(bodyPart);
                if (null != a) {
                    inventory.add(a);
                }
            }
        }
    }

    public class Chars {

        public String name;
        public int level;
        public String gender;
    }

}

Originally it looked something like this (old version so there are less fields than now):

package com.game.quest.core.domain;

import java.util.ArrayList;
import java.util.HashMap;
import java.util.List;
import java.util.Map;
import java.util.Map.Entry;


public class Player {

    private String name;
    private int level;
    private String gender;

    private List<Stat> stats = new ArrayList<>();
    private List<Skill> skills = new ArrayList<>();
    private List<Title> titles = new ArrayList<>();
    private Map<String, Quest> quests = new HashMap<>();

    public void setName(String name) {
        this.name = name;
    }

    public String getGender() {
        return gender;
    }

    public void setGender(String gender) {
        this.gender = gender;
    }

    public String getName() {
        return name;
    }

    public void setLevel(int level) {
        this.level = level;
    }

    public int getLevel() {
        return level;
    }

    public Title getTitle(int i) {
        return titles.get(i);
    }

    public int numberOfTitles() {
        return titles.size();
    }

    public int numberOfSkills() {
        return skills.size();
    }

    public Skill getSkill(int i) {
        return skills.get(i);
    }

    public int numberOfQuests() {
        return quests.size();
    }

    public int numberOfStats() {
        return stats.size();
    }

    public Stat getStat(int i) {
        return stats.get(i);
    }

    public void addSkill(Skill skill) {
        skills.add(skill);
    }

    public void addStat(Stat stat) {
        stats.add(stat);
    }

    public List<Quest> getQuests() {
        return new ArrayList<>(quests.values());
    }

    public Quest getQuest(String name) {
        return quests.get(name);
    }

    public void addQuest(Quest quest) {
        quests.put(quest.getName(), quest);
    }

    public Map<String, Quest> filterQuestStatus(String filter) {
        Map<String, Quest> filtered = new HashMap<>();
        for(Entry<String, Quest> e: quests.entrySet()) {
           if (e.getValue().getStatus().equals(filter)) {
               filtered.put(e.getKey(), e.getValue());
           }
        }
        return filtered;
    }
}

I know I've done a bad thing, but was it worth it?

\$\endgroup\$
  • \$\begingroup\$ I know I've done a bad thing...But was it worth it? What exactly do you want to have reviewed if the bad parts are on purpose? \$\endgroup\$ – Mast Jul 7 '15 at 10:07
  • \$\begingroup\$ I know that public for the fields is considered bad practice. But I believe in that case it was necessary evil. Having dozens of getters/setters + methods that wrap basic functionality of collections and etc. is worse than making field public. \$\endgroup\$ – lapots Jul 7 '15 at 10:10
  • \$\begingroup\$ I only use getters/setters if I need to run code when getting or setting values, or other developers are going to be working on something and I want to make properties read only. I know this will get frowned upon by some but I've never heard a reason to do anything different, other than preference. \$\endgroup\$ – Reinstate Monica Cellio Jul 7 '15 at 10:35
  • \$\begingroup\$ @Archer Maintainability. \$\endgroup\$ – Mast Jul 7 '15 at 10:42
  • \$\begingroup\$ @Mast I believe I covered that scenario when I mentioned other developers working on the same code. If I know for a fact that it's just me then I do what I want, and getters/setters are not necessarily relevant. \$\endgroup\$ – Reinstate Monica Cellio Jul 7 '15 at 10:50
5
\$\begingroup\$

Using public fields is considered rather immoral, but I can feel your pain. I'm using project Lombok, which takes care of them. But your problem is different: you need no getters and setters, but a way to deal with your lists and maps and Lombok won't help you with them much (it'd help slightly and I'd use it anyway).

public class Player {

    public PlayerData is;
    public Chars crs;

    {
        is = new PlayerData();
        crs = new Chars();
    }

Initializer blocks are rather confusing and safe you nothing when compared to a constructor.

But actually, this should be

public class Player {
    public PlayerData playerData = new PlayerData();
    public Chars chars = new Chars();

Variable names like is are terrible at best.

But still... now class Player contains public mutable fields. So someone may get the field and modified it and nothing happens as someone else replaced them like in

someone: playerData.stats.clear();
someoneElse: playerData = new PlayerData();

You're offering multiple ways how to do something and that's just bad. So make all mutable members final.


    public Map<String, Armor> armor = new HashMap<>();

Use final.

        public List<Armor> getArmor() {
            return new ArrayList<>(armor.values());
        }

This makes little sense to me. You're having Map, which anyone can use at will and provide a List returning and copying getter. That's too confusing.

I'd for sure prefer some getter-setter-boilerplate to having a confusing class.

  • Either it's just a stupid container and then public fields could be acceptable assuming final on all mutable parts (*)
  • or it's a class doing something interesting and then all fields should be private

(*) With Lombok, I'd made them private final and use @Getter. At the very least, I'd get something I could put a breakpoint on.

| improve this answer | |
\$\endgroup\$
  • \$\begingroup\$ Well...There is no someone else...I am all alone doing it. I know what I change and where. I just want to get rid of tons of work. \$\endgroup\$ – lapots Jul 7 '15 at 11:09
  • 5
    \$\begingroup\$ @user1432980 There's always "someone else" even when working alone. In a single-person project I had to ask many times "what idiot is doing this"? \$\endgroup\$ – maaartinus Jul 7 '15 at 11:14
  • \$\begingroup\$ What's confusing about an initializer block? \$\endgroup\$ – dennisdeems Jul 7 '15 at 19:51
  • \$\begingroup\$ @dennisdeems Everybody expects variables to be initialized inline or in the constructor, don't they? Doing something else is therefore confusing. It's surely OK to use initializer blocks when you have no other choice (anonymous classes or multiple constructors which can't be chained). \$\endgroup\$ – maaartinus Jul 7 '15 at 22:02
4
\$\begingroup\$

I think what Disco Beat is getting at (and very rightfully so) was that from a object oriented mindset you are approaching this problem incorrectly. I would recommend looking up and researching the difference between mutable and immutable objects if you are unsure of what they are. That said the point I want to get in (which was already slightly covered) is to the idea of hiding the details of a class and offering solutions to manipulate data. I often think of the car illustration: How do you calculate fuel level? Do you first put on a new gas tank, then set the number of (gallons/litres) then do the math? Or do you look at your fuel gauge? Many examples on the internet would have you do the first. Instead you should just offer a means to know how much fuel remains. The code difference would be (bad)

public class FuelTank {
    private double remainingFuel;
    private final double fuelCapacity = 15.0;

    //getter/setter for remaining fuelCapacity
    //getter for fuelCapacity    
}

or a more OO approach which can easily be tested

public class FuelTank {
    private double remainingFuel;
    private final double fuelCapacity = 15.0;

    public void consumeFuel(double fuelUsed){
        if (fuelUsed > remainingFuel) 
            remainingFuel = 0;
        else
            remainingFuel -= fuelUsed;
    }

    public double getFuelRemiainingPercentage(){
        return ((remainingFuel/fuelCapacity)/100.0);
    }
}

(Note that I did this in Notepad++ so syntax might not be 100% as it is for example only) Now with your stats. Take a step back and consider what it means to put in a getter and setter in a player for his stats. If Conan levels up and has an increase in stats you're giving yourself multiple ways of increasing his stats. You could new up a set of stats and set that. You could increase the stats manually on each property. The problem with multiple ways of doing something is that it allows for subtle bugs that can't be covered in tests. Lets say for instance you want to have fireworks go off when Conan goes up a level. You would put in a listener for when the level changes, but what if you new up all that? The level would technically not have changed and therefor the fireworks never go off. Now Conan destroys your monitor because you didn't give him due reverence. Consider the next 3 methods. The first two are how a person could increase Conan's level, and the third is a potential way that you could implement. Which one gives the least amount of reasons to change?

public void IncreasePlayersLevel1(Player player){
    player.crs.level++;
}

public void IncreasePlayersLevel2(Player player){
    Chars newChars = new Chars();
    newChars.name = player.crs.name;
    newChars.level = player.crs.level + 1;
    newChars.gender = player.crs.gender;

    player.crs = newChars;
}

public void IncreasePlayersLevel3(Player player) {
    player.changeLevel(1);
}

The 3rd way is the safest, as it presents to the user a way to change levels with barely any reason to change. If you take that concept and make methods on your player or your stats that change the data according to game play rules that will limit reasons for your code to change. This is called the open-closed principle: Code should be open for extension, but closed for change. The less you have to change your code the less ways there are for you to introduce new bugs, and it will be easier to extend your code to do new cool things, which is always a win.

To point out an area in your code that is close, the armor methodology is almost on the right track. But again take a step back. Why does anyone need to know what armor Conan wearing? Is it for when taking damage? Is it for showing the name of the armor on the screen? The first can be easily fixed by making the Player class "Accept" damage. If a Vanir raider hits Conan for 100 hit-points your player class can then use its knowledge of the armor to adjust how much damage was taken to the player. Speaking of extensibility you could then use that same method to let Conan drink a potion of health to regain his hit points with no change to your code! That is what I'm getting at. It is common to see people blindly put up getters and setters without thinking about a more OO approach.

| improve this answer | |
\$\endgroup\$
3
\$\begingroup\$

This is not something recommended by most of the programmers out there.

If you know you are doing something breaking a rule and have a really good reason to do it, I see no shame in doing it. But yours is not a good one to me.

You could also define classes Stats, Skills, Titles like this :

public class Stats {
   private List<stat> stats;

   // ctor etc.

   // methods to manipulate the list
   public Stat Get(int i) { ... }
   // etc.
}

So your consumers won't know about the internal structure and will deal with meaningfull objects.

Your class Player will look like :

public class Player {
   private String name;
   private int level;
   private String gender;

   private Stats stats = new Stats();
   private Skills skills = new Skills();
   private Titles titles = new Titles();
   private Quests = new Quests();

   // getters and setters for name etc.

   // getters and setters for others
   public Stats GetStats();
   public Skills GetSkills();
   public Titles GetTitles();

   // and/or if you really want to access it from this class from outside
   public Stat GetStat(int i) {
       return this.stats.Get(i);
   }
}

Now, whatever happen to your Stats, Skills etc. classes, as long as you keep the interface untouched, you don't have to modify their client code.

| improve this answer | |
\$\endgroup\$
  • \$\begingroup\$ But I don't like that approach - I mean I have to divide my class into several additional classes instead of making them modular and more or less independent. \$\endgroup\$ – lapots Jul 7 '15 at 10:55
  • \$\begingroup\$ You want the player class to be responsible for all the logic? \$\endgroup\$ – disco beat Jul 7 '15 at 10:58
  • \$\begingroup\$ I want it to be responsible for that belong to player. I have additional classes like PlayerUtils with some logic like calculateDefence for the player. \$\endgroup\$ – lapots Jul 7 '15 at 11:03
  • 2
    \$\begingroup\$ Having meaningfull classes describing concepts is a good idea :) Why do you think that the number of classes is important? \$\endgroup\$ – disco beat Jul 7 '15 at 11:22
  • 1
    \$\begingroup\$ You are the only one to know :) I would say that if they are related, it's not a bad choice to have them grouped and easily accessible by a getter. By doing this, you are uncoupling your code and your consumers. \$\endgroup\$ – disco beat Jul 7 '15 at 11:41
3
\$\begingroup\$

You are questioning your use of public and private when, as others have pointed out, you have bigger problems to deal with. I have personally never liked the use of the 'private' keyword in Java (though it is not as bad as 'final') and while that is a debate for another day, this is an example of one of the reasons I don't like it; It is making you think about who will access your data instead of how. DiscoBeat had some ideas that you didn't like because you wanted "modular and more or less independent" classes. Why? In what way do you think that would make them better?

Straight away I would say your player has a set of attributes that is incomplete and will be expanded in the future. Attributes that have as yet undefined characteristics and behaviours that will vary between attributes in question and the context they are used, so an accessor like Player.getGender() really does not make sense and if it doesn't do the job, who really cares if it is public or private?

I would suggest you invest some time in looking at design patterns, I recommend the "head first design patterns" book as an excellent primer. It wont answer all your questions, but hopefully it will get you asking the right ones.

| improve this answer | |
\$\endgroup\$
2
\$\begingroup\$

I know I've done a bad thing, but was it worth it?

You are clearly mistaken here, what you think is a 'bad' thing - public fields - is not the worst culprit here, but the fact that you have non-final mutable fields as pointed out rightfully by @maaartinus. The following simple class is an example where getters/setters are rightfully redundant (ignoring the lack of equals()/hashcode()).

public final class Point {
    public final int x;
    public final int y;

    public Point(int x, int y) {
        this.x = x;
        this.y = y;
    }
}

Dealing with mutable objects such as your ArrayList or HashMap is trickier, as you probably even want to limit any external modifications to their internal state. This is where getters and setters are even more crucial in providing such protection within your codebase.

But the number of methods started to grow, I used lists and maps so additionally I had to create methods to get size of them and every item by index, key and etc.

Without going too much into a design-focused answer to what seems like a design-driven question, what I can suggest here is to share with us bits of your codebase that requires such look-ups... perhaps there's an XY-problem worth reviewing/solving.

| improve this answer | |
\$\endgroup\$
0
\$\begingroup\$

Having public fields instead of getters/setters is no big deal. They work almost the same. The problem is to encapsulate your logic in your class. If anyone has access to those variables, they can play with them from outside the encapsulation. If you're working on a single project by yourself and have the discipline not to break encapsulation then everything's fine to me.

| improve this answer | |
\$\endgroup\$
  • 3
    \$\begingroup\$ Is your viewpoint standards compliant? When someone posts on Code Review they normally want their code to be better. If you are suggesting something that isn't going to be widely accepted then it isn't going to be better code. \$\endgroup\$ – Malachi Jul 7 '15 at 15:15

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.