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

initial problem generator module #39

Merged
merged 4 commits into from
Feb 6, 2025
Merged

Conversation

ATOMiNATiON
Copy link
Owner

No description provided.

Copy link
Collaborator

@s5bug s5bug left a comment

Choose a reason for hiding this comment

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

I recommend making some changes, but they aren't really required. 👍

Comment on lines 24 to 29
let num = this.generateNumber(digits, true);
result.push(num);

if (result.reduce((sum, n) => sum + n, 0) <= 0) {
result.pop();
}
Copy link
Collaborator

Choose a reason for hiding this comment

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

I would suggest instead replacing all these checks by

  1. Keeping a running sum separate to result
  2. Passing -theRunningSum to generateNumber as something like furthestNegativeAllowed
  3. Rejecting negation if the number it generates is less than furthestNegativeAllowed

Copy link
Collaborator

Choose a reason for hiding this comment

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

I.e., keeping this method O(n) rather than unbounded if you get mega unlucky.

Copy link
Owner Author

Choose a reason for hiding this comment

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

let me know if you see any issues with the updated one.

return [num1, num2];
}

divide(digits1: number, digits2: number): number[] {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Perhaps the result of this, rather than number[], should be some sort of ProblemSet class with an entries: number[] field and a result: number field.

@ATOMiNATiON ATOMiNATiON merged commit cc76912 into main Feb 6, 2025
1 check passed
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.

4 participants