Document purpose of ten high complexity functions

[shrew97]

"You identify ten functions/methods with high complexity, and document the purpose of them, and why the complexity should be high (or not)."

Will later on be added to report.

[shrew97]

Function: Monarch::actionIsValid CCN according to lizard: 19 CCN according to us: 5 (see issue #34) Purpose: Checks if a specified action is valid for a Monarch at the current game time. Complexity analysis: I believe that this function does not do too much or too little. It only has a switch statement in it that covers every possible value the parameter can have (considering it's an enum). If there would be something to change, then it would be to remove the empty cases which could reduce the CCN slightly and use the default case instead (which right now will never be used). Documentation: This function is not very well documented for the different outcomes, but has a brief description of what it does together with a description of the argument and return value.

[johanmoritz]

Function: TerrainGenerator::createLandRegions Purpose: Takes a map of a continuous stretch of land, and divides it into a number of regions depending on how the map is defined. Complexity analysis: createLandRegions is complex because it tries to heavily optimize a lot of different processes by running them together and avoiding unnecessary data copying between functions. The function itself is inherently complex because it does a lot of different steps. A way to reduce the complexity would needlessly sacrifice a bit of speed for a smaller a more succinct function.

[shrew97]

Function: Player::readChild CCN according to lizard: 22 CCN according to us: 21 (see issue #33) Purpose: Reads a player object, based of a tag, from an xml file and then creates it. Complexity analysis: This function is basically a bunch of branches that go through as many (if not all?) cases as possible of what the tag in the xml file could be. These will be difficult to reduce the complexity of, but there are inner if-statements that could be moved out (to perhaps the create function called) as they do a little bit more than just read the object and create it. It also handles cases such as nulls, which could be moved out. Documentation: Not a lot of documentation regarding the different outcomes, but the code that is run during each of these is not too much and is pretty straight forward so it is still possible to get a grasp of what's going on.

[shrew97]

Function: Scope::equals CCN according to lizard: 18 CCN according to us: 5 (see issue #53) Purpose: Compares two scope objects to each other Complexity analysis: Compares the relevant fields to each other to check that these two objects are in fact equal. However, also does some null checks which could be handled in the get-ers for said fields. This would remove that logic from the function completely, which makes sense considering that it should only compare the values and not have to handle nulls. Documentation: Not a lot of documentation for the different outcomes, but then again it's an equals function and doesn't do too much interesting. Could however add documentation regarding which fields are of importance, null checks etc.

[Kappenn]

Function: BaseCostDecider::getCost CNN according to lizard:: 24 CNN according to manuel count: 19 Purpose::Determines the cost of a single move. Complexity analysis: The function takes four arguments: (final Unit unit, final Location oldLocation, final Location newLocation, int movesLeftBefore)

• Unit: the unit making the move
• oldLocation: the location we are moving from
• newLocation: the location we are moving to
• movesLeftBefore: The moves left before making the move
• Return: the cost of moving a unit from oldLocation to newLocation

The function compares different moves based on what unit type, terrain, new location and old location, determines the cost for the move. The code is easy to follow and together with the documentation quite self evident.

The reason for the high CCN for the function is mainly the many switch statements and if statements, it is hard to decrease the CCN without moving out some of the methods. This function was refactored in #57.

[rbratfors]

Function: ModelMessage::getDefaultDisplay CCN according to lizard: 22 CCN according to manual count: 21 Purpose: Returns the display object of a given message type. Complexity analysis: The function takes two parameters messageType and source, messageType is of course the given message type and source is what the display will be taken from. The function is basically a big switch statement and most of the cases are fall through. Some cases even fall through to the default case which increases the complexity redundantly.

[Kappenn]

Function: Monarch::initializeCaches CCN according to lizard: 25 CNN according to manuel count: 19 and 22 Purpose: Cache the unit types and roles for support and mercenary offers. Complexity analysis: Structurize unit in a neat way based on their abilities and group them together. Depending on the type of unit based Navel, Bombard, Land, Mercenary, the units get put into a group of corresponding class. The code is not very well documented, but it is quite clear what it does.

The function consist of if-else statement and therefore hard to improve the CCN without moving some of the methods to outside the function.

[adbjo]

Function: Map::findMapPath CCN according to lizard: 23 Purpose: Finds a path on the map for a unit, from start tile to end tile. Complexity analysis: The function takes the parameters unit, start, end, carrier, costDecider, and lb (LogBuilder). It has several if statements. The main one checks whether the start and end tile are contiguous (belong to the same part of the map). The other if statements checks attributes of the unit and the tiles, such as:

• water/land units
• water/land tiles
• carrier units
• unit is on carrier

It makes sense that the function is complex since pathfinding is a complex task. The function is not overly complex since its sole purpose is to set the parameters of the main pathfinding function called searchMap, which is much more complex. Documentation is clear.

[adbjo]

Function: Unit::setLocation CCN according to lizard: 20 CCN calculated manually: 15 Purpose: Sets the location for a Unit instance. Complexity analysis: This is a setter method in the Unit class for the location field. It only takes the parameter newLocation, which is the value the field should be set to. In the game, units belong to colonies depending on which colony their location belongs to. Other attributes of the unit also depends on which colony they are in. All this is handled by this method, which cause the high complexity. We have not attempted to refactor this function but an idea which could reduce complexity is to move this colony logic to the Colony class. There is, however, a comment stating the following:

// We have to handle this issue here in setLocation as this is
// the only place that contains information about both
// locations.

So there seems to have been some thought in handling the colony logic here. Documentation is clear.

[adbjo]

Function: Player::writeChildren CCN according to lizard: 22 Purpose: Writes Player attributes (children) to a XML file using the FreeColXMLWriter class. Complexity analysis: The function converts each field in the Player class to XML and writes to a stream (FreeColXMLWriter). The complexity is high since it contains a bunch of if statements that check whether certain fields exists, and loops for fields that are Collections. We have not refactored this but it would probably be a good idea to move much of this logic to the FreeColXMLWriter class. For example, adding a writeField method (check if field != null and then writes it), and a writeCollection method (writes a collection/list of values to the stream). In its current state the method is unnecessarily complex. Documentation is clear in the FreeColXMLWriter class, the method in the Player class lacks documentation.