Address PR review feedback: fix indentation, resource leak, test naming (#361)

* Address PR review comments: fix indentation, resource leak, test naming, and documentation

---------

Co-authored-by: copilot-swe-agent[bot] <198982749+Copilot@users.noreply.github.com>
Co-authored-by: bedaHovorka <5263405+bedaHovorka@users.noreply.github.com>

Author: Copilot

src/jmh/kotlin/cz/vutbr/fit/interlockSim/benchmarks/GridStoragePerformance.kt

@@ -31,57 +31,57 @@ import kotlin.random.Random
 @Fork(value = 2, jvmArgs = ["-ea"])
 open class GridStoragePerformance {
 
-    // Railway grid dimensions for simulation
-    private val gridDimension = 1000
-    
-    // Test data: coordinates to lookup in grid
-    private lateinit var testCoordinates: List<Point>
-    
-    // Storage implementations to benchmark
-    private lateinit var customGridMap: Array2DMap<Int>
-    private lateinit var standardTreeMap: TreeMap<Point, Int>
+	// Railway grid dimensions for simulation
+	private val gridDimension = 1000
 
-    @Setup(Level.Trial)
-    fun prepareTestData() {
-        // Generate consistent test coordinates using seeded random
-        val coordinateGenerator = Random(42)
-        testCoordinates = List(gridDimension) {
-            Point(
-                coordinateGenerator.nextInt(gridDimension),
-                coordinateGenerator.nextInt(gridDimension)
-            )
-        }
-        
-        // Initialize Array2DMap with test data
-        customGridMap = Array2DMap()
-        testCoordinates.forEachIndexed { idx, coord ->
-            customGridMap[coord] = idx
-        }
-        
-        // Initialize TreeMap with same data and comparator
-        standardTreeMap = TreeMap(Array2DMap.POINT_COMPARATOR)
-        testCoordinates.forEachIndexed { idx, coord ->
-            standardTreeMap[coord] = idx
-        }
-    }
+	// Test data: coordinates to lookup in grid
+	private lateinit var testCoordinates: List<Point>
 
-    @Benchmark
-    fun measureCustomGridLookup(): Int {
-        // Test Array2DMap performance for railway grid cell access
-        var checksum = 0
-        for (coord in testCoordinates) {
-            checksum += customGridMap[coord] ?: 0
-        }
-        return checksum
-    }
+	// Storage implementations to benchmark
+	private lateinit var customGridMap: Array2DMap<Int>
+	private lateinit var standardTreeMap: TreeMap<Point, Int>
 
-    @Benchmark
-    fun measureStandardTreeLookup(): Int {
-        // Baseline: TreeMap performance for comparison
-        var checksum = 0
-        for (coord in testCoordinates) {
-            checksum += standardTreeMap[coord] ?: 0
-        }
-        return checksum
-    }
+	@Setup(Level.Trial)
+	fun prepareTestData() {
+		// Generate consistent test coordinates using seeded random
+		val coordinateGenerator = Random(42)
+		testCoordinates = List(gridDimension) {
+			Point(
+				coordinateGenerator.nextInt(gridDimension),
+				coordinateGenerator.nextInt(gridDimension)
+			)
+		}
+
+		// Initialize Array2DMap with test data
+		customGridMap = Array2DMap()
+		testCoordinates.forEachIndexed { idx, coord ->
+			customGridMap[coord] = idx
+		}
+
+		// Initialize TreeMap with same data and comparator
+		standardTreeMap = TreeMap(Array2DMap.POINT_COMPARATOR)
+		testCoordinates.forEachIndexed { idx, coord ->
+			standardTreeMap[coord] = idx
+		}
+	}
+
+	@Benchmark
+	fun measureCustomGridLookup(): Int {
+		// Test Array2DMap performance for railway grid cell access
+		var checksum = 0
+		for (coord in testCoordinates) {
+			checksum += customGridMap[coord] ?: 0
+		}
+		return checksum
+	}
+
+	@Benchmark
+	fun measureStandardTreeLookup(): Int {
+		// Baseline: TreeMap performance for comparison
+		var checksum = 0
+		for (coord in testCoordinates) {
+			checksum += standardTreeMap[coord] ?: 0
+		}
+		return checksum
+	}
 }

src/main/kotlin/cz/vutbr/fit/interlockSim/gui/MenuBar.kt

@@ -87,8 +87,10 @@ class MenuBar : JMenuBar() {
 							frame.setContext(context)
 							frame.modificationTracker.setCurrentFile(selectedFile)
 							frame.modificationTracker.markClean()
+						} else {
+							// User cancelled - close context to avoid resource leak
+							context.close()
 						}
-						// If CANCEL, do nothing (file remains closed)
 					}
 
 					// Case 3: Unparseable XML (malformed syntax) - show error and block

src/test/kotlin/cz/vutbr/fit/interlockSim/sim/TrainReverseDirectionTest.kt

@@ -27,47 +27,55 @@ import org.koin.test.inject
 private val logger = KotlinLogging.logger {}
 
 /**
- * Integration tests for Train.reverseDirection() functionality.
+ * Unit tests for Timetable.reverseDirection() functionality.
  *
  * ## Purpose
  *
- * These tests verify that trains can reverse their direction of travel during simulation,
- * simulating the train engineer moving to the opposite end of the train.
+ * These tests verify the timetable reversal logic that supports bidirectional
+ * train operation. The reversal swaps the In/Out destinations and is used by
+ * Train.reverseDirection().
  *
  * ## Test Scenarios
  *
  * Uses vyhybna.xml network configuration:
- * - Train reverses direction when stopped
- * - Validates preconditions (train must be stopped)
- * - Verifies In/Out destinations are swapped
- * - Tests that reversed train can complete journey
+ * - Timetable reverses In/Out destinations
+ * - Validates destination swap is correct
+ * - Verifies other timetable properties remain unchanged
+ * - Tests that reversed timetable is usable for train creation
+ *
+ * ## Note on Train.reverseDirection()
+ *
+ * Train.reverseDirection() includes additional simulation behavior (hold(30.0)
+ * to simulate engineer movement) that requires running simulation. That full
+ * integration test scenario requires stopping a train mid-simulation in jDisco,
+ * which is complex to test. This test focuses on the core timetable reversal
+ * logic that Train.reverseDirection() delegates to.
  *
  * ## Conservative Approach
  *
  * Per CLAUDE.md guidance for sim/ package:
  * - Uses existing vyhybna.xml network (realistic topology)
- * - Short simulation times (30-60 seconds)
- * - Validates train state without modifying Train class internals
- * - Tests observe behavior through public APIs only
+ * - Tests core logic without running full simulation
+ * - Validates state through public APIs only
  *
  * ## Railway Context
  *
  * This feature simulates a simplified version of locomotive coupling/uncoupling:
  * - In reality, only possible with certain modern train types
  * - Simulation allows this for any train for flexibility
- * - Engineer movement delay (30s) provides realistic timing
+ * - Engineer movement delay (30s) in Train.reverseDirection() provides realistic timing
  *
  * @since 2026-02-06 (GitHub #62)
  */
 @Tag("integration-test")
-@DisplayName("Train Reverse Direction - Integration Tests")
+@DisplayName("Timetable Reverse Direction - Unit Tests")
 class TrainReverseDirectionTest : KoinTestBase() {
 	private val simulationContextFactory: SimulationContextFactory by inject()
 	private lateinit var context: DefaultSimulationContext
 
 	@BeforeEach
 	fun setUp() {
-		logger.info { "Loading vyhybna.xml for train reverse direction testing" }
+		logger.info { "Loading vyhybna.xml for timetable reverse direction testing" }
 
 		// Load vyhybna.xml - realistic railway network
 		val xml =
@@ -78,20 +86,19 @@ class TrainReverseDirectionTest : KoinTestBase() {
 
 		context = simulationContextFactory.createContext(xml) as DefaultSimulationContext
 
-		logger.info { "Train reverse direction test setup complete" }
+		logger.info { "Timetable reverse direction test setup complete" }
 	}
 
 	@AfterEach
 	fun tearDown() {
-		// Only stop if simulation was actually started
-		// These tests don't run the simulation, just test the API
+		// These tests don't run the simulation, just test the timetable API
 		if (::context.isInitialized) {
-			logger.info { "Train reverse direction test teardown complete" }
+			logger.info { "Timetable reverse direction test teardown complete" }
 		}
 	}
 
 	@Test
-	fun `train can reverse direction when stopped`() {
+	fun `timetable can reverse In and Out destinations`() {
 		// Arrange
 		val inOuts = context.getInOuts().toList()
 		assertThat(inOuts.size >= 2).isEqualTo(true)
@@ -114,20 +121,13 @@ class TrainReverseDirectionTest : KoinTestBase() {
 		assertThat(timetable.getIn()).isEqualTo(inPoint)
 		assertThat(timetable.getOut()).isEqualTo(outPoint)
 
-		// Act - Activate train but don't start simulation yet
-		// We'll test the reverseDirection method directly
-		// This is a unit-style test within an integration test framework
-
-		// Since we can't easily stop a train mid-simulation in jDisco without
-		// running the full simulation, we'll test the method when train is in
-		// initial stopped state (velocity = 0)
+		// Act - Test the timetable reversal logic directly
+		// Note: Train.reverseDirection() calls this same method, but also includes
+		// hold(30.0) simulation delay which requires running simulation context
 		assertThat(train.getVelocity()).isEqualTo(0.0)
-
-		// Reverse direction (this will use hold() internally, so we need to activate as Process)
-		// For this test, we'll verify the timetable swap happened correctly
 		timetable.reverseDirection()
 
-		// Assert
+		// Assert - Verify In/Out swap
 		assertThat(timetable.getIn()).isEqualTo(outPoint)
 		assertThat(timetable.getOut()).isEqualTo(inPoint)
 	}

src/test/kotlin/cz/vutbr/fit/interlockSim/xml/XMLContextFactoryLenientTest.kt

@@ -62,14 +62,14 @@ class XMLContextFactoryLenientTest : KoinTestBase() {
 	@DisplayName("Lenient parsing - Parseable XML with validation errors")
 	inner class ParseableWithErrorsTests {
 		@Test
-		fun singleInOut_shouldReturnParseableWithErrors() {
-			// Arrange: single-inout.xml has only 1 InOut (violates minimum 2 requirement)
+		fun singleInOut_shouldBeValidAfterBidirectionalSupport() {
+			// Arrange: single-inout.xml has only 1 InOut (meets minimum 1 requirement)
 			val fixtureFile = getFixtureFile("single-inout.xml")
 
 			// Act
 			val result = xmlContextFactory.createContextLenient(fixtureFile)
 
-			// Assert: Should be parseable but have validation errors
+			// Assert: Should be parseable and valid (minimum 1 InOut is now allowed)
 			assertThat(result.isParseable).isTrue()
 			assertThat(result.context).isNotNull()
 			assertThat(result.validationResult.isValid).isTrue()

src/test/kotlin/cz/vutbr/fit/interlockSim/xml/XMLPolishTest.kt

@@ -416,7 +416,7 @@ class XMLPolishTest : KoinTestBase() {
 		}
 
 		@Test
-		fun `exactly one InOuts is valid`() {
+		fun `exactly one InOut is valid`() {
 			val xml = """<?xml version="1.0"?>
 				<!DOCTYPE net>
 				<net X="100" Y="100">