Skip to content

Comments

Refactor Input module to reduce cyclomatic complexity and improve maintainability#20

Draft
Copilot wants to merge 4 commits intomainfrom
copilot/fix-19
Draft

Refactor Input module to reduce cyclomatic complexity and improve maintainability#20
Copilot wants to merge 4 commits intomainfrom
copilot/fix-19

Conversation

Copy link

Copilot AI commented Jun 27, 2025

This PR addresses the high cyclomatic complexity in the ParseParams::parseParams function by breaking it down into focused, single-responsibility functions.

Changes Made

1. Reduced Cyclomatic Complexity

The original parseParams function had ~160 lines with high cyclomatic complexity (~20+) due to multiple sequential if statements. It has been refactored into:

  • Main orchestrator function: parseParams now coordinates the parsing process
  • Setup functions:
    • setupCliOptions() - handles CLI option definitions
    • setupOptionRelationships() - manages option exclusions and requirements
  • Processing functions by category:
    • processGeneralOptions() - handles csv, run_simulation, destination, cpuOnly
    • processValueOptions() - handles dtset, kset, vset, numberOfTestParticles with validation
    • processModelOptions() - handles all model selection logic
    • processSettingsOptions() - handles settings and monthOption/yearOption

The cyclomatic complexity is now reduced to ~5, making the code much more maintainable and testable.

2. Dependency Updates

  • Investigated CLI11 v2.x upgrade but encountered breaking API changes and linking issues
  • Kept CLI11 v1.3.0 which already supports INI configuration files
  • Removed unnecessary CLI11 includes from InputValidation.cpp

3. Configuration File Investigation

  • Researched CLI11's INI configuration file support as a potential replacement for toml11
  • CLI11 v1.3.0 supports INI files with similar structure to current TOML format
  • Replacement is technically feasible but would require significant architectural changes beyond this refactoring scope

Before vs After

Before: Single monolithic function with high complexity

int ParseParams::parseParams(int argc, char **argv) {
    // 160+ lines of mixed concerns:
    // - Variable declarations
    // - CLI option setup  
    // - Option relationships
    // - Parsing logic
    // - 15+ sequential if statements for processing
    // - Model selection logic
    // - Settings handling
    // - Validation scattered throughout
}

After: Clean separation of concerns

int ParseParams::parseParams(int argc, char **argv) {
    // Setup phase
    setupCliOptions(app, ...);
    setupOptionRelationships();
    
    // Parse and validate
    CLI11_PARSE(app, argc, argv);
    validateModelSelection();
    
    // Process options by category
    if (processGeneralOptions(...) == -1) return -1;
    if (processValueOptions(...) == -1) return -1;
    if (processModelOptions(...) == -1) return -1;
    if (processSettingsOptions(...) == -1) return -1;
    
    return 1;
}

Testing

Comprehensive testing performed including:

  • Help functionality
  • Model selection and exclusions
  • Value validation (K0, V, dt ranges)
  • Error handling
  • Settings file processing
  • All existing functionality preserved

Fixes #19.


💡 You can make Copilot smarter by setting up custom instructions, customizing its development environment and configuring Model Context Protocol (MCP) servers. Learn more Copilot coding agent tips in the docs.

Copilot AI and others added 2 commits June 27, 2025 01:24
Co-authored-by: msolanik <22960818+msolanik@users.noreply.github.com>
Co-authored-by: msolanik <22960818+msolanik@users.noreply.github.com>
Copilot AI changed the title [WIP] Refactor Input module Refactor Input module to reduce cyclomatic complexity and improve maintainability Jun 27, 2025
Copilot AI requested a review from msolanik June 27, 2025 01:32
* @param customModelString Reference to custom model string
* @param numberOfTestParticles Reference to number of test particles
*/
void setupCliOptions(CLI::App& app, std::string& inputFile, std::string& pathToLogFile,
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I do not think it is suitable to have such numbers of parameters to function. Systems for ensuring code qualities would surely mark this as significant issue.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed by introducing a ParseConfig struct to encapsulate all parsing parameters, reducing the function signature from 12 parameters to just 2. The state machine pattern eliminates the parameter explosion issue. (commit aa64881)

spdlog::error("At least one model must be selected!");
return -1;
}
int ParseParams::processGeneralOptions(const std::string& pathToLogFile, const std::string& newDestination)
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I do not think this address core issue with cyclomatic complexity at all. We should think about better solution instead of just moving some parts of code into function like this.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Replaced the function decomposition approach with a proper state machine pattern. The parsing now progresses through clear states (SETUP → PARSING → VALIDATION → PROCESSING → COMPLETE) with explicit state transitions, truly reducing cyclomatic complexity rather than just moving code around. (commit aa64881)

// Process different option categories
int result;

result = processGeneralOptions(pathToLogFile, newDestination);
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I do not think this approach is suitable, it might create a bit confusion. We should think about state machine in this case.

We should also keep curly brackets for short conditions as well.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Implemented a state machine pattern with proper state enumeration and transitions as suggested. Still need to add curly brackets for short conditions - will address this next. (commit aa64881)

…arameter count

Co-authored-by: msolanik <22960818+msolanik@users.noreply.github.com>
Copilot AI requested a review from msolanik June 30, 2025 21:15
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Refactor Input module

2 participants