Skip to content

Fix/issue-575: Handling no experimentId in mark api call#584

Merged
mfugate1 merged 2 commits intodevfrom
fix/issue-575
Oct 27, 2022
Merged

Fix/issue-575: Handling no experimentId in mark api call#584
mfugate1 merged 2 commits intodevfrom
fix/issue-575

Conversation

@ppratikcr7
Copy link
Copy Markdown
Contributor

No description provided.

Comment on lines 270 to 271
if (experimentDecisionPoint.length && experimentId !== '') {
let selectedExperimentDP = experimentDecisionPoint.filter( dp => dp.experiment.id === experimentId);
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Just wondering if it's a better solution than let selectedExperimentDP = experimentDecisionPoint.filter( dp => dp.experiment.id === experimentId || experimentId === ''); which was what I did to not see the error.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Both will give same result, but in your case it will check again for the dp which already we can filter out before as I have done and we just return the monitored dp.

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Got it, thank you!

@ppratikcr7 ppratikcr7 disabled auto-merge October 27, 2022 13:01
@ppratikcr7 ppratikcr7 requested a review from zackcl October 27, 2022 13:22
@danoswaltCL
Copy link
Copy Markdown
Collaborator

danoswaltCL commented Oct 27, 2022

If this is really all that's needed, then the tertiary condition to set the empty string really isn't necessary, we can simplify a little more:

if (!experimentId) {
   if (filteredExperiments.length > 1) {
      const random = seedrandom(userId)();
      experimentId = filteredExperiments[Math.floor(random * experiments.length)].id;
   } else {
      experimentId = filteredExperiments[0]?.id; // <--- no need for extra conditional here, let it be undefined, no need for empty string
   }
}

and then you can do this so that it's not just an empty string it's checking, it's checking all falsey values that would potentially get the same runtime error:

if (experimentDecisionPoint.length && experimentId) {

which also catches any weird edge-case where experimentId is somehow falsey for any other reason (such as if somehow experimentId = filteredExperiments[Math.floor(random * experiments.length)].id; returns undefined or NaN, which could technically happen I think if experiments.length is undefined or an unexpected number, or if that id is messed up. These aren't important scenarios, but you never know.

@ppratikcr7
Copy link
Copy Markdown
Contributor Author

If this is really all that's needed, then the tertiary condition to set the empty string really isn't necessary, we can simplify a little more:

if (!experimentId) {
   if (filteredExperiments.length > 1) {
      const random = seedrandom(userId)();
      experimentId = filteredExperiments[Math.floor(random * experiments.length)].id;
   } else {
      experimentId = filteredExperiments[0]?.id; // <--- no need for extra conditional here, let it be undefined, no need for empty string
   }
}

and then you can do this so that it's not just an empty string it's checking, it's checking all falsey values that would potentially get the same runtime error:

if (experimentDecisionPoint.length && experimentId) {

which also catches any weird edge-case where experimentId is somehow falsey for any other reason (such as if somehow experimentId = filteredExperiments[Math.floor(random * experiments.length)].id; returns undefined or NaN, which could technically happen I think if experiments.length is undefined or an unexpected number, or if that id is messed up. These aren't important scenarios, but you never know.

Okay, I will update the PR. Thanks :)

@mfugate1 mfugate1 enabled auto-merge (squash) October 27, 2022 15:57
@mfugate1 mfugate1 merged commit af4c33d into dev Oct 27, 2022
@mfugate1 mfugate1 deleted the fix/issue-575 branch October 27, 2022 17:19
danoswaltCL pushed a commit that referenced this pull request Oct 27, 2022
* handling no experimentId in mark api call

* updated the optional experimentId in mark
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.

/mark unhandled error: "TypeError: Cannot read property 'id' of undefined"

4 participants