Skip to content

fix function of DateToDay and the tests #1122

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.

Sign up for GitHub

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

Jump to bottom
Closed
wants to merge 1 commit into from
Closed

fix function of DateToDay and the tests #1122

wants to merge 1 commit into from

Conversation

rajprogrammerbd
Copy link

This pull request solves the issue of #1121

appgurueu
appgurueu requested changes Oct 5, 2022
View reviewed changes
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.

The entire purpose of this repo is to implement the algorithms for learning purposes. Just using the builtin Date library fails this purpose entirely.

@appgurueu appgurueu added the invalid Doesn't seem right label Oct 5, 2022
@appgurueu appgurueu closed this Oct 5, 2022
@sumantpaldm
Copy link

my team has solved this problem without using the date feature
please consider our solution too.

@sumantpaldm
Copy link

we have used totally different approach to fix this issue

chrisjwaddell
chrisjwaddell reviewed Oct 5, 2022
View reviewed changes
Copy link

@chrisjwaddell chrisjwaddell left a comment

Choose a reason for hiding this comment

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

month/day/year would create confusion, I suggest (day, month, year) or an object {{year: ...., month: ....., day: ......})
Also, there is no checking like the original function:
if (day < 0 || day > 31 || month > 12 || month < 0) {

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

@chrisjwaddell chrisjwaddell chrisjwaddell left review comments

@appgurueu appgurueu appgurueu requested changes

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

Assignees
No one assigned
Labels
invalid Doesn't seem right
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@rajprogrammerbd @sumantpaldm @appgurueu @chrisjwaddell