Adventure game player using public fields instead of getters and setters

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 stats = new ArrayList<>();
public List skills = new ArrayList<>();
public List titles = new ArrayList<>();
public List inventory = new ArrayList<>();
public Map quests = new HashMap<>();
public Map armor = new HashMap<>();
public Map weapons = new HashMap<>();
public Map accessories = new HashMap<>();

class PlayerDataHelper {

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

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

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

public voi

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 armor = new HashMap<>();

Use final.

public List 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.

StackExchange Code Review Q#96068, answer score: 5