terryebdon / trk21 Goto Github PK

There's a lot of Quadrant and Galaxy manipulation in Trek. This has resulted
in a very large class that can be confusing. Creating new QuadrantManagement
and GalaxyManagement classes would simplify the things.

Trek.positionKlingons() is badly named. It does far more than its name implies, and the actual positioning of enemies occurs in trek.positionEnemy(). The commented-out code should also be removed.

This looks like an off-by-one error:

assert key > 0 && key < devices.size()
       |   |   |  |   | |       |
       6   true|  6   | |       6

at net.ebdon.trk21.DamageControl.inflictDamage(DamageControl.groovy:121)

i.e. it should be <= device.size(). It would be better to use a Range for
this, e.g.

assert key in 1..devices.size()

The wrapper method was added as part of an earlier refactoring. The wrapper is not needed, and can be eliminated with Remove Middle Man. The corresponding configuration option, in TrekGameConfig, can also be removed.

Firing a torpedo should:

1. Display a track until either:

• it hits the target, or
• it leaves the sector.

2. Remove the target from the game if the torpedo hits it, where a target can be any game object, i.e.

• An enemy ship.
• A star base.
• A star.

4. Tell the user they've won the game when the last target is destroyed.

Note: Some of these items may be implemented as part of issue #3.

The testGame() method is very slow, e.g. nearly two and a quarter seconds on an old laptop.
See also issue #18, as I suspect it has the same cause. i.e. Trek is a sub-class of LoggingBase.

jcenter is now obsolete. This breaks clean builds, as the dependencies will no longer download.

Prerequisite for Issue #47.

When "test playing" the game it would save time if the tester could bypass the guess-work required to navigate to a particular sector or quadrant, without resorting to a scientific calculator. A "cheat" or "wizard" mode should be implemented to allow access to this feature. it should be possible to enable and disable the feature via cofinguration.

The quadrant setup code is should be moved out of Trek,groovy, as the Trek class is far too large. Extracting this setup code would make app easier to test and maintain.

Most interaction is localised, but there are some significant ommisions. Resolving these will require some changes to the UI interface classes. The test classes will also need changes, to consistently verify resource bundle usage and not depend on unlocalised messages.

DamageControl holds a reference to the Trek.damage map. This map is directly referenced by:

• Trek.spaceStorm()
• Trek.setCourse()
• Trek.longRangeScan()
• TrekLongRangeScanTest.testScanDamaged()

The damage map should be a private member of DamageControl.

Most of the long range scanning logic is in the main Trek class. It would make more sense for this functionality to be in its own class.

A separate log configuration for test was added by issue #16. The two log configurations are now completely independent of each other. However some configuration is now duplicated, e.g. the log file location and file pattern. It should be possible to move common configuration into a single file.

See also issue #33.

In order to extract the Repositioner class from Trek the new class was given a
reference to Trek. This has resulted in Repositioner being too dependent on
Trek.

The JaCoCo plugin was applied to the build to address issue #19 - Report test coverage. While this works well it is extremely slow. This is apparently due to the instrumentation and analysis (particularly the latter), even when a HTML report is not generated.

Unit tests without JaCoCo plugin

C:\projects\Trk21>gradlew test --rerun-tasks

> Task :test
BUILD SUCCESSFUL in 55s
5 actionable tasks: 5 executed

Unit tests with JaCoCo plugin

C:\projects\Trk21>gradlew test --rerun-tasks

> Task :test
BUILD SUCCESSFUL in 2m 7s
5 actionable tasks: 5 executed

This method currently has a complexoty value of 33, which is far too high.
See also #24 - Trek.isValid() is overly complex.

When docked the star base will always protect the ship from enemy fire and top-up the ship's energy. Under which circumstances should a docked ship's energy be exhausted?

  STARDATE:  2608        CONDITION: DOCKED
  QUADRANT: 5 - 7           SECTOR: 5 - 7
    ENERGY:  3000  PHOTON TORPEDOS: 10
  KLINGONS:    19

Command: p
  Number of units to fire: 3000
  Phasers locked in on target. Energy available 3000
  1137 unit hit on Enemy ship No. 1 at sector 3 - 2
  Enemy ship destroyed!
  3000 unit hit on Enemy ship No. 2 at sector 6 - 8
  Enemy ship destroyed!
  987 unit hit on Enemy ship No. 3 at sector 8 - 1
  Enemy ship destroyed!
  It is stardate 2608

 Your ship has been destroyed after 8 years.
 Your civilisation will be conquered.
 There are still 16 enemy ships.
 You are dead.

This should be possible with the Commons Logging Bridge.

The Language resource bundle is located via ClasspathResourceManager. This should not be needed; it would be better to use ResourceBundle.getBundle

It's possible that the existing code is not respecting the user's locale settings. 🐞

See also: #33

Trying to travel through a star base is shown as an impact. Stopping next to a base is docking. But the ship is always docked after an impact. This is possibly confusing.

  ---------------
  . . . . * . . .
  . . . . . . . .
  . . . . . . . .
  . . . . . . . .
  . . . . . . . .
  . . . . . . . *
  . E . . . . . B
  . . . . . . . .
  ---------------
  STARDATE:  2414        CONDITION: GREEN
  QUADRANT: 2 - 7           SECTOR: 2 - 7
    ENERGY:   877  PHOTON TORPEDOS: 10
  KLINGONS:    21

Command: c
  Course (1-8.99999): 1
  Warp Factor (0-12): 1
  Ship blocked by base at sector [7, 8] == 8 - 7
  ---------------
  . . . . * . . .
  . . . . . . . .
  . . . . . . . .
  . . . . . . . .
  . . . . . . . .
  . . . . . . . *
  . . . . . . E B
  . . . . . . . .
  ---------------
  STARDATE:  2415        CONDITION: DOCKED
  QUADRANT: 2 - 7           SECTOR: 7 - 7
    ENERGY:  3000  PHOTON TORPEDOS: 10
  KLINGONS:    21

Use a static code analyser, e.g. CodeNarc, to improve code quality.

The original code performs a short range scan after a course move. My Groovy code does not do this automatically; it should.

Output from original code, running on an emulated PDP-11 via SIMH V39-0

COMMAND? C
COURSE (1-8.99999)? 1
WARP FACTOR (0-12)? 3


---------------
* . * . . . . .
. . . . . . . .
. . E * . . . .
. . . . . . . .
. . . . . . . .
. . B * . . . .
. . . . . . . .
. . . . . . . .
---------------

STARDATE: 3501           CONDITION: YELLOW
QUADRANT: 4 - 6          SECTOR: 3 - 3
ENERGY: 2976             PHOTON TORPEDOS: 10
KLINGONS: 21

COMMAND? ^C

Ready

Code of the form:

log.<level> sprintf( template, args )

Can often be replaced with:

log.<level> template, args

The latter approach is preferred.

This test just adds one to an integer and checks the result against TrekCalendar.tick(). Why does a simple test, involving only integer addition and comparison, take nearly 3.5 seconds to run?

When short range sensors are damaged they still responds to an 'S' command, and still display output following course moves.

Command: c
  Course (1-8.99999): 1
  Warp Factor (0-12): .5
  *** Space Storm, device.S.R..SENSORS damaged ***
  ---------------
  . . . . . . . E
  . B . . . . . *
  . . . . . * . .
  . . * . . . . .
  . . . . . . . .
  . . . * . . . .
  . . . . . . . .
  . . . . . . . .
  ---------------
  STARDATE:  2812        CONDITION: YELLOW
  QUADRANT: 7 - 2           SECTOR: 8 - 1
    ENERGY:  2064  PHOTON TORPEDOS: 10
  KLINGONS:    33

Command: c
  Course (1-8.99999): 5
  Warp Factor (0-12): .75
  Repair systems are working on damage to device.S.R..SENSORS, state improved to -3
  ---------------
  . E . . . . . .
  . B . . . . . *
  . . . . . * . .
  . . * . . . . .
  . . . . . . . .
  . . . * . . . .
  . . . . . . . .
  . . . . . . . .
  ---------------
  STARDATE:  2813        CONDITION: DOCKED
  QUADRANT: 7 - 2           SECTOR: 2 - 1

Most of the existing unit tests use JUnit style asserts It would be better to use Grovvy's power assertions as:

1. Power assertion provide more informative error messages.
2. It reduces coupling to JUnit.

Currently the ship's condition is held in a string member variable which is assigned with / checked against various hard-coded values. This makes localisation awkward and requires additional code to check that only allowed values are used. replacing the string with a class / enum would simplify the code. This is also a prerequisite for issues #33 and #37.

Firing a phaser should:

1. Reduce the target's available energy by the hit amount.
2. Remove the target from the game if it's energy <= 0.
3. Inform the user of the weapon's effectiveness.
4. Tell the user they've won the game when the last target is destroyed.

Note: Some of these items may be deferred until torpedo firing is implemented, or moved into their own enhancement.

Changing log levels has a dramatic effect on the run time of a gradle check command. e.g. on an old & slow laptop it can vary between 28 seconds, at ERROR level, and over 4 minutes, at TRACE level. Separating the test and main log configuration will resolve this.

Log performance and noisiness should also be reviewed, under a separate issue. It would also good to use common logging configuration between test and main where possible; another issue will be raised for this.

Currently Trek.longRangeScan() is difficult to test. It might be better to move it into Galaxy, or a dedicated class.

Trek.isValid() is overly complex. It should be simpler and clearer.
Note: There is a similar problem in FederationShip.isValid().

JaCoCo shows a complexity of 24. See the JaCoCo report for the current values.

The repeated use of != null should be eliminated by using the ?. operator. isValid() should be replaced by valid. Screen shot is from the JaCoCo detail report.

There are currently 58 implemented tests, but test coverage is patchy. Implementing code-coverage reporting would provide a bench-mark.

Energy use should be easier to determine from the log. With the log at level debug and using a filter of (fire)|(energy)|(unit), with the Atom language-log package, I can see course changes correctly modifying energy levels, but changes due to damage / enemy-fire are less obvious.

Currently the player's ship will never have its energy reduced due to a hit from an enemy attack. See also enhancement issue #8. The problem is apparently due to Battle.phaserAttackFleet() passing an empty fleetAttackReporter closure to enemyFleet.attack() See this line in Battle.groovy.

See also issue #42.

While docked the ship should be protected from all enemy attacks, but this is not always the case. The enemy fleet will attack under two conditions:

1. Immediately before executing a user-requested maneuver.
2. As a counter-attack in response to phaser fire or a torpedo launch.

In scenario 1 the ship is protected, and this prevents the fleet from attacking. A check in Trek.enemyAttacksBeforeShipCanMove() enables this protection.

In scenario 2 the battle computer initiates a fleet counter-attack without asking the ship if it is protected. This is a bug. A fix is required in Battle.enemyCounterAttacks().

The message appears every time a short-range scan is performed:

Using key: [8, 4]
  ---------------
  . . . . . . . .
  . . . . . . . .
  . . . . . . . *
  . . . . . . . .
  . . . . . . . .
  * . . . . . . .
  . * . . . . . .
  . E . . . . . .
  ---------------
  STARDATE:  3600        CONDITION: YELLOW
  QUADRANT: 4 - 8           SECTOR: 2 - 8
    ENERGY:  3000  PHOTON TORPEDOS: 10
  KLINGONS:    23

Looks like the culprit is line 38 in Galaxy.groovy.

• RpositionerTestBase references TrekMock
• TrekMock references FederationShipMock

It should be relatively easy to eliminate the FederationShipMock class. Getting
rid of TrekMock may be more difficult, due to the coupling between trek and
Repositioner.

1. Maximum warp factor is reduced to 0.2 when the engines are damaged. But ShipVector rejects any WarpFactor < 0.25. As the ship is only repaired during course moves this prevents the ship from ever moving again.
2. A warp factor of 0.25 moves the ship by 2 sectors (horizontally or vertically, less for diagonals.) To cross one sector a warp factor of 0.125 is required. i,e, it's currently not possible to make these short moves.

Command: s
  ---------------
  . . * . . . . .
  . . . . . . . .
  . . . . E . . *
  . . . . . B . .
  . . . . . . . .
  . . . . . . . .
  . . . . . . . .
  . . . . . . . .
  ---------------
  STARDATE:  3116        CONDITION: DOCKED
  QUADRANT: 4 - 5           SECTOR: 5 - 3
    ENERGY:  3000  PHOTON TORPEDOS: 10
  KLINGONS:    17

Command: c
  Course (1-8.99999): 7
  Warp Factor (0-12): .2

Command: s
  ---------------
  . . * . . . . .
  . . . . . . . .
  . . . . E . . *
  . . . . . B . .
  . . . . . . . .
  . . . . . . . .
  . . . . . . . .
  . . . . . . . .
  ---------------
  STARDATE:  3116        CONDITION: DOCKED
  QUADRANT: 4 - 5           SECTOR: 5 - 3
    ENERGY:  3000  PHOTON TORPEDOS: 10
  KLINGONS:    17

Command:

Trk21 is build with Groovy version 2.5.8,, the latest Groovy release is 4.0.6. This is not a "drop-in" replacement, as some Groovy classes have changed or moved. e.g. AntBuilder. New versions of Jacoco and CodeNarc will be required. Also the Gradlew wrapper will have to be updated.

From: build.gradle:

    implementation 'org.codehaus.groovy:groovy-all:2.5.8'

Depends on issue #48.

This is odd as the code looks OK, from a quick glance. (Ignore the hard-coded messages and keys, they're logged as seperate issues.)

Despite the above test a warp factor of .2 is rejected:

This may be related to issue #6 - Minimum warp factor should be 0.125, not 0.25, though that issue is closed.