Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly
Sign up
Appearance settings

add MinMax game theory JS #1193 commented out test case #1278

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

Closed
devhiteshk wants to merge 11 commits into TheAlgorithms:master from devhiteshk:master
Closed

add MinMax game theory JS #1193 commented out test case #1278

devhiteshk wants to merge 11 commits into TheAlgorithms:master from devhiteshk:master

Conversation

Copy link

@devhiteshk devhiteshk commented Jan 13, 2023

Open in Gitpod know more

Describe your change:

  • [x ] Add an algorithm?
  • [x ] Fix a bug or typo in an existing algorithm?
  • [ x] Documentation change?

Checklist:

  • [x ] I have read CONTRIBUTING.md.
  • [x ] This pull request is all my own work -- I have not plagiarized.
  • [x ] I know that pull requests will not be merged if they fail the automated tests.
  • [ x] This PR only changes one algorithm file. To ease review, please open separate PRs for separate algorithms.
  • [x ] All new JavaScript files are placed inside an existing directory.
  • [ x] All filenames should use the UpperCamelCase (PascalCase) style. There should be no spaces in filenames.
    Example:UserProfile.js is allowed but userprofile.js,Userprofile.js,user-Profile.js,userProfile.js are not
  • [x ] All new algorithms have a URL in its comments that points to Wikipedia or other similar explanation.
  • [x ] If this pull request resolves one or more open issues then the commit message contains Fixes: #{$ISSUE_NO}.

Copy link
Author

Dear Maintainers please have a look when you get active :)

Copy link
Collaborator

@appgurueu appgurueu left a comment

Choose a reason for hiding this comment

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

You have various unrelated changes in there, including bumping some dependencies.

Copy link
Author

I think now it is fine

@devhiteshk devhiteshk changed the title (削除) add MinMax game theory JS #1193 commented out test case and .at(index) fix for not passing tests (削除ここまで) (追記) add MinMax game theory JS #1193 commented out test case (追記ここまで) Jan 13, 2023
Copy link
Author

You have various unrelated changes in there, including bumping some dependencies.

All conflicts have now been resolved

Copy link
Collaborator

@appgurueu appgurueu left a comment •
edited
Loading

Choose a reason for hiding this comment

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

Consider refactoring out the class. In this example, a simple function should suffice. If you don't want to repeat some calculations, consider a simple function containing a recursive closure:

export default function solveMinMax(scores, ...) {
 const height = Math.log2(scores.length)
 const solve = (...) => {
 ...
 ...(solve(...), solve(...))
 ...
 }
 return solve(...)
}

Furthermore, what exactly is the merit of this? In its current form, this:

  • Needs to have all scores evaluated beforehand anyways, because they must be provided in an array;
  • Always recurses into both branches.

Consider implementing lazily evaluating scores. I'd suggest passing a function to get a score for a node rather than an array which needs to be fully evaluated. This can also help in making this more flexible.

test('if the number is greater or equal to 2167', () => {
expect(problem44(2167)).toBe(8476206790)
})
// test('if the number is greater or equal to 2167', () => {
Copy link
Collaborator

@appgurueu appgurueu Jan 14, 2023

Choose a reason for hiding this comment

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

This unrelated change still doesn't belong here.

import { MinMax } from '../MinMax'

describe('MinMax', () => {
it('should return 65 for MinMax([90, 23, 6, 33, 21, 65, 123, 34423],0,0,true)', () => {
Copy link
Collaborator

@appgurueu appgurueu Jan 14, 2023

Choose a reason for hiding this comment

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

This test is practically equivalent to the below test. You're also duplicating the contents in the description. Please see the TypeScript guide for writing good tests.

}
}

// MinMax(scores, depth, nodeindex, ismax)
Copy link
Collaborator

@appgurueu appgurueu Jan 14, 2023

Choose a reason for hiding this comment

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

This outcommented code ought to be removed.

// >>> minimax(0, 0, True, scores, this.height)
// 12

class MinMax {
Copy link
Collaborator

@appgurueu appgurueu Jan 14, 2023

Choose a reason for hiding this comment

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

Why not prefix this with export (perhaps even export default) rather than exporting below?

// 12

class MinMax {
constructor (scores, depth, nodeIndex, isMax) {
Copy link
Collaborator

@appgurueu appgurueu Jan 14, 2023

Choose a reason for hiding this comment

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

This constructor is completely superfluous - you're passing all these as arguments to solve later on. I'm questioning whether this should be a class at all - IMO it should just be a single solve function that checks it parameters and recurses.

)
}

get_ans () {
Copy link
Collaborator

@appgurueu appgurueu Jan 14, 2023

Choose a reason for hiding this comment

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

Not in lowerCamelCase

}

get_ans () {
this.answer = this.solve(
Copy link
Collaborator

@appgurueu appgurueu Jan 14, 2023

Choose a reason for hiding this comment

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

Storing the answer in the class is dirty IMO; why not just return it?

this.answer = -1
}

solve (depth, nodeIndex, isMax, scores, height) {
Copy link
Collaborator

@appgurueu appgurueu Jan 14, 2023

Choose a reason for hiding this comment

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

You don't have to pass all of these as parameters - you already have them as instance variables.

*
*/

// nodeIndex is index of current node in scores[].
Copy link
Collaborator

@appgurueu appgurueu Jan 14, 2023

Choose a reason for hiding this comment

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

This comment is messy. There are no Python-like doc comments in this repo.

@@ -0,0 +1,86 @@
/*
*
* Min Max problem
Copy link
Collaborator

@appgurueu appgurueu Jan 14, 2023

Choose a reason for hiding this comment

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

Please make this a proper JSDoc comment.

Copy link
Author

I think I can't code :(

Copy link
Collaborator

Don't worry, your code seems to be a correct port of the code found on Geeks for Geeks. It's just that the Geeks for Geeks code is poorly structured (IMO).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@appgurueu appgurueu appgurueu requested changes

@raklaptudirm raklaptudirm Awaiting requested review from raklaptudirm raklaptudirm is a code owner

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

AltStyle によって変換されたページ (->オリジナル) /