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

refactor for tautological condition non-nil not equal to nil #126

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

sejima1105
Copy link
Contributor

Description

Refactoring for tautological condition non-nil != nil

Checklist

Please check if applicable

  • Tests have been added (if applicable, ie. when operator codes are added or modified)
  • Relevant docs have been added or modified (if applicable, ie. when new features are added or current features are modified)

Relevant issue https://github.com/st-tech/sre-dept-issues/issues/7361

Operation check

I ran the sample scenario and confirmed that no errors occurred.

@sejima1105 sejima1105 requested a review from a team September 9, 2024 07:22
@@ -373,7 +373,7 @@ func doNotRequeue(err error) (ctrl.Result, error) {

func (r *GatlingReconciler) createVolumesForCR(ctx context.Context, gatling *gatlingv1alpha1.Gatling, namespace string, log logr.Logger) error {
// Create Simulation Data ConfigMap if defined to create in CR
if &gatling.Spec.TestScenarioSpec.SimulationData != nil && len(gatling.Spec.TestScenarioSpec.SimulationData) > 0 {
if gatling.Spec.TestScenarioSpec.SimulationData != nil && len(gatling.Spec.TestScenarioSpec.SimulationData) > 0 {
Copy link
Contributor

Choose a reason for hiding this comment

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

When gatling is nil, the nil pointer access is panicked, so here is the case

if gatling ! = nil && gatling.Spec.TestScenarioSpec.SimulationData && len(gatling.Spec.TestScenarioSpec.SimulationData) > 0

is a good choice here.
The zero value of map is nil, so the nil check of SimulationData is left as it is.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thank you for the review!

It is indeed a very good idea.
However, I thought that using this approach might change the existing behavior if gatling is nil.
For example, if gatling is nil in the createVolumesForCR function, no processing is performed and error is returned as nil.

To prevent this, I think it would be better to check for nil at the beginning of the function and return an error at that point.
What do you think?

Here's what I have in mind to add at the beginning of the function.

if gatling == nil {
    return errors.New("gatling is nil")
}

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.

2 participants