Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

move watches config to env #231

Merged
merged 1 commit into from
Jan 18, 2025
Merged

move watches config to env #231

merged 1 commit into from
Jan 18, 2025

Conversation

sveneld
Copy link
Collaborator

@sveneld sveneld commented Jan 18, 2025

PR Type

enhancement, configuration changes


Description

  • Moved watches configuration from hardcoded values to environment variables.

  • Updated service bindings to use new environment variables for watches configuration.

  • Refactored classes to remove dependency on Configuration and use injected parameters.

  • Updated templates and logic to align with new configuration structure.


Changes walkthrough 📝

Relevant files
Configuration changes
3 files
config.example.php
Removed hardcoded watches configuration.                                 
+0/-9     
services.php
Updated service bindings for watches configuration.           
+20/-2   
.env.dist
Added watches configuration environment variables.             
+21/-1   
Enhancement
6 files
LongRentalCheckCommand.php
Refactored to use injected long rental hours.                       
+5/-6     
UserController.php
Removed unused configuration dependency.                                 
+0/-2     
HomeController.php
Added free time hours parameter to controller.                     
+2/-0     
TooManyBikeRentEventListener.php
Refactored to use injected parameters for time and number limits.
+9/-7     
AbstractRentSystem.php
Adjusted logic to use watches configuration parameters.   
+2/-1     
index.html.twig
Updated free time logic to use injected parameter.             
+1/-1     

Need help?
  • Type /help how to ... in the comments thread for any question about Qodo Merge usage.
  • Check out the documentation for more information.
  • Copy link
    Contributor

    PR Reviewer Guide 🔍

    Here are some key observations to aid the review process:

    ⏱️ Estimated effort to review: 2 🔵🔵⚪⚪⚪
    🧪 No relevant tests
    🔒 No security concerns identified
    ⚡ Recommended focus areas for review

    Code Readability

    The nested if conditions and calculations in the rental fee logic could be simplified and extracted into separate methods for better readability and maintainability

    if ($this->creditSystem->getPriceCycle() && $timediff > $this->watchesConfig['freetime'] * 60 * 2) {
        // after first paid period, i.e. freetime*2; if pricecycle enabled
        $temptimediff = $timediff - ($this->watchesConfig['freetime'] * 60 * 2);
        if ($this->creditSystem->getPriceCycle() == 1) { // flat price per cycle
            $cycles = ceil($temptimediff / ($this->watchesConfig['flatpricecycle'] * 60));
            $creditchange = $creditchange + ($this->creditSystem->getRentalFee() * $cycles);

    Copy link
    Contributor

    PR Code Suggestions ✨

    Explore these optional code suggestions:

    CategorySuggestion                                                                                                                                    Score
    General
    Extract complex rental fee calculation logic into a dedicated method to improve code maintainability and readability

    Add a comment explaining the complex rental fee calculation logic and consider
    extracting the calculation into a separate method for better readability and
    maintainability.

    src/Rent/AbstractRentSystem.php [394-399]

    -if ($this->creditSystem->getPriceCycle() && $timediff > $this->watchesConfig['freetime'] * 60 * 2) {
    -    // after first paid period, i.e. freetime*2; if pricecycle enabled
    +private function calculateRentalFee(int $timediff): float {
    +    // Skip calculation if price cycle is disabled or within free time period
    +    if (!$this->creditSystem->getPriceCycle() || $timediff <= $this->watchesConfig['freetime'] * 60 * 2) {
    +        return 0;
    +    }
    +    
    +    // Calculate fee for time exceeding the free period
         $temptimediff = $timediff - ($this->watchesConfig['freetime'] * 60 * 2);
    -    if ($this->creditSystem->getPriceCycle() == 1) { // flat price per cycle
    +    if ($this->creditSystem->getPriceCycle() == 1) {
             $cycles = ceil($temptimediff / ($this->watchesConfig['flatpricecycle'] * 60));
    -        $creditchange = $creditchange + ($this->creditSystem->getRentalFee() * $cycles);
    +        return $this->creditSystem->getRentalFee() * $cycles;
    +    }
    +    return 0;
    +}
    • Apply this suggestion
    Suggestion importance[1-10]: 7

    Why: The suggestion improves code maintainability by extracting complex rental fee calculation logic into a dedicated method with clear responsibility and better structure. The improved code also adds better error handling and documentation.

    7

    @sveneld sveneld merged commit 5ccdf6e into main Jan 18, 2025
    2 checks passed
    @sveneld sveneld added this to the Symfony Migration milestone Jan 18, 2025
    Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
    Projects
    None yet
    Development

    Successfully merging this pull request may close these issues.

    1 participant