0
\$\begingroup\$

This is a class representing a city in a mod I am creating for Minecraft. All the constructor parameters are mandatory and they are used to help the class that calculates and builds the Paths, Buildings etc.

The City class has all the fields that are listed in the constructor including a 2D array that is a representation of the city in X and Z axes. There are also 2 methods to serialize and deserialize the data in order to save them using the Minecraft Forge API.

public City(int id, BlockPos startingPos, int citySize, int edgeLenght, int pathExtends, Block groundBlock, Block pathBlock, boolean hasMainStreets, boolean hasPaths) {
    this.id = id;

    this.startingPos = startingPos;

    this.chunkLength = citySize;

    this.cityLength = citySize * 16;
    this.edgeLength = edgeLenght;
    this.mapLength = cityLength + edgeLenght + blockStart;

    this.pathExtends = pathExtends;

    this.hasMainStreets = hasMainStreets;
    this.hasPaths = hasPaths;

    this.groundBlock = groundBlock;
    this.pathBlock = pathBlock;

    this.blockStart = edgeLenght;

}

Initializing a city object looks like:

BlockPos startingPos = new BlockPos(1000,63,1000);
int citySize = 5; // Will be pseudorandom
int edgeLength = 4;// Will be pseudorandom
int pathExtends = 2;// Will be pseudorandom
City newCity = new City(++id, startingPos, citySize, edgeLength, pathExtends, Blocks.OBSIDIAN, Blocks.DIAMOND_BLOCK, true, true);

From an organizational point of view I can see the benefits of this approach but is this a good practice? Does it involve any pitfalls I have not thought about or is there a better way to do this?

This is not easy to read and understand and I was wondering if there was any better way to do it. My idea was to create CitySettings object hence making more readable the City object by moving all these fields to another class.

I am thinking of creating a class CitySettings to hold all of City's fields and an instance of CitySettings that will be stored as a field in the City class.

\$\endgroup\$
4
  • 6
    \$\begingroup\$ I don't think that we can give you meaningful advice for improving this code by analyzing it in isolation. Please provide more code for context. See How to Ask. \$\endgroup\$ – 200_success Oct 7 '19 at 14:22
  • 1
    \$\begingroup\$ Note you have a typo in edgeLenght. \$\endgroup\$ – dfhwze Oct 7 '19 at 18:45
  • 2
    \$\begingroup\$ I think you're still missing enough of the code that this is hard to review - you mention methods for (de)serializing the data, but don't provide them. Can you also provide a link to some documentation on the API for those of us who are unfamiliar with it? \$\endgroup\$ – Dan Oberlam Oct 7 '19 at 19:17
  • \$\begingroup\$ I am not providing more code because I do not think it is relevant to what I am asking a code review with and the rest of the class consists of getters for the fields of the class. \$\endgroup\$ – Vpant Oct 7 '19 at 19:26
1
\$\begingroup\$

Use the builder pattern. The builder object can remain unmentioned, and City (maybe) without public constructor:

City varna = City.create()
    .withStartingPos(0)
    .withCitySize(10_000)
    .build();

Where City.create() returns a CityBuilder. Checks can be done in the final CityBuilder.build method raising an ' IllegalStateException("Missing mapLength and groundBlock").

This also allows some fields to be final (immutable).

Such data classes - when stored in a database table - might also profit from a record format version, indicator of which fields were defined, as one might expect some changes in the fields.

If those data have no calculatory use, you were right to store them is a map / CitySettings.

\$\endgroup\$
1
\$\begingroup\$

If it's readability you're mostly worried about, then you could change the style of your code:

public City(int id,          BlockPos startingPos,
            int citySize,    int edgeLenght, 
            int pathExtends, Block groundBlock,
            Block pathBlock, boolean hasMainStreets,
            boolean hasPaths) {
    this.id             = id;
    this.startingPos    = startingPos;
    this.chunkLength    = citySize;
    this.cityLength     = citySize * 16;
    this.edgeLength     = edgeLenght;
    this.mapLength      = cityLength + edgeLenght + blockStart;
    this.pathExtends    = pathExtends;
    this.hasMainStreets = hasMainStreets;
    this.hasPaths       = hasPaths;
    this.groundBlock    = groundBlock;
    this.pathBlock      = pathBlock;
    this.blockStart     = edgeLenght;

}

Columnising is a useful trick which can make things more readable.

Yes, there is still visibly a lot of parameters clogging up the screen, but it's not so much that you can't put it all in one constructor. It also helps to remember KISS (Keep It Simple, Stupid), don't overcomplicate unless you need to.

\$\endgroup\$
1
\$\begingroup\$

Regarding your idea of creating a CitySettings class: from introducing such a class, you'd have only moved the problem - now you have a CitySettings constructor with 9 constructors. Not an improvement.

Further possibilities: if a significant number of the fields are optional, create a builder-pattern. (Do not do this, if all parameters are mandatory, as an object should be completely usable after creation.)

If you find a meaningful subset of the parameters (e.g. startingPos, size, edgeLength might be somehing like GeographicalBounds) you can create a class for these and reduce the parameter count.

\$\endgroup\$

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