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

[PLA-1806] Class init error #46

Merged
merged 3 commits into from
Jun 13, 2024
Merged

[PLA-1806] Class init error #46

merged 3 commits into from
Jun 13, 2024

Conversation

leonardocustodio
Copy link
Member

@leonardocustodio leonardocustodio commented Jun 7, 2024

PR Type

Bug fix


Description

  • Fixed a potential class initialization error by making the FuelTank property nullable and initializing it to null.

Changes walkthrough 📝

Relevant files
Bug fix
FuelTankCreated.php
Make `FuelTank` property nullable and initialize to null 

src/Services/Processor/Substrate/Events/Implementations/FuelTanks/FuelTankCreated.php

  • Changed FuelTank property to be nullable.
  • Initialized FuelTank property to null.
  • +1/-1     

    💡 PR-Agent usage:
    Comment /help on the PR to get a list of all available PR-Agent tools and their descriptions

    Copy link

    github-actions bot commented Jun 7, 2024

    PR Review 🔍

    ⏱️ Estimated effort to review [1-5]

    2, because the PR involves a simple change in a single file with a clear purpose, making it relatively straightforward to review.

    🧪 Relevant tests

    No

    ⚡ Possible issues

    Possible Null Reference: Making FuelTank nullable could lead to null reference exceptions elsewhere in the code if other parts of the codebase assume the fuelTankCreated property is always non-null.

    🔒 Security concerns

    No

    Code feedback:
    relevant filesrc/Services/Processor/Substrate/Events/Implementations/FuelTanks/FuelTankCreated.php
    suggestion      

    Consider adding a check for null before using the fuelTankCreated property in methods that depend on it. This will prevent potential null reference exceptions that could occur due to this change. [important]

    relevant lineprotected ?FuelTank $fuelTankCreated = null;

    Copy link

    github-actions bot commented Jun 7, 2024

    PR Code Suggestions ✨

    CategorySuggestion                                                                                                                                    Score
    Best practice
    Move initialization of properties to the constructor for better clarity and management

    Consider initializing the fuelTankCreated property within the constructor of the class
    rather than at the point of declaration. This approach enhances clarity by centralizing
    initialization logic and can help manage dependencies more effectively if the
    initialization depends on constructor parameters in the future.

    src/Services/Processor/Substrate/Events/Implementations/FuelTanks/FuelTankCreated.php [21]

    -protected ?FuelTank $fuelTankCreated = null;
    +protected ?FuelTank $fuelTankCreated;
     
    +public function __construct() {
    +    $this->fuelTankCreated = null;
    +}
    +
    Suggestion importance[1-10]: 7

    Why: Moving the initialization of the fuelTankCreated property to the constructor can improve code clarity and maintainability by centralizing initialization logic. However, the current initialization at the point of declaration is also a valid approach, and the improvement is relatively minor.

    7

    Copy link
    Contributor

    @enjinabner enjinabner left a comment

    Choose a reason for hiding this comment

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

    some comment

    @leonardocustodio leonardocustodio merged commit e8033c3 into master Jun 13, 2024
    5 checks passed
    @leonardocustodio leonardocustodio deleted the PLA-1806v5 branch June 13, 2024 17:48
    Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
    Development

    Successfully merging this pull request may close these issues.

    3 participants