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

Fix DateToDay #1126

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
sharansukesh1003 wants to merge 10 commits into TheAlgorithms:master from sharansukesh1003:master
Closed

Conversation

Copy link

@sharansukesh1003 sharansukesh1003 commented Oct 5, 2022 •
edited by appgurueu
Loading

Fix implementation of Zeller's congruence

refer to https://www.geeksforgeeks.org/zellers-congruence-find-day-date/

Open in Gitpod know more

Describe your change:

  • Fix a bug or typo in an existing algorithm?

Checklist:

  • I have read CONTRIBUTING.md.
  • This pull request is all my own work -- I have not plagiarized.
  • I know that pull requests will not be merged if they fail the automated tests.
  • This PR only changes one algorithm file. To ease review, please open separate PRs for separate algorithms.
  • All new JavaScript files are placed inside an existing directory.
  • 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
  • All new algorithms have a URL in its comments that points to Wikipedia or other similar explanation.
  • If this pull request resolves one or more open issues then the commit message contains Fixes: #{$ISSUE_NO}.

@appgurueu appgurueu added the fix Fixes a bug label Oct 6, 2022 
The test case '18/02/2001' and '01/01/2001' were Invalid. I have corrected them and also attached the calendar link for reference
(https://www.timeanddate.com/calendar/?year=2001&country=35)
First time contribution, sorry for the inconvenience.
@appgurueu appgurueu linked an issue Oct 7, 2022 that may be closed by this pull request
11: 9,
12: 10
}

// show the week day in a number : Sunday - Saturday => 0 - 6
const daysNameList = { // weeks-day
Copy link
Collaborator

@appgurueu appgurueu Oct 7, 2022

Choose a reason for hiding this comment

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

Please use an array here.

// check the data are valid or not.
if (day < 0 || day > 31 || month > 12 || month < 0) {
return new TypeError('Date is not valid.')
}
// divide year to century and yearDigit value.
const yearDigit = (year % 100)
// date is resolved based on Zeller's congruence.
Copy link
Collaborator

@appgurueu appgurueu Oct 7, 2022

Choose a reason for hiding this comment

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

That's the overarching comment. This should be specific to the following line: "Count Jan & Feb as months 13 & 14 of the previous year".

Copy link
Author

@sharansukesh1003 sharansukesh1003 Oct 7, 2022

Choose a reason for hiding this comment

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

Ohhh great input, I will change it.

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.

Of all proposed PRs I prefer this so far as it gets rid of calcMonthList (which is a dict rather than a list)

 

Updated the comment and renamed the daysNameList to daysNameDict. If there is any changes to be done please let me know, if not then please accept my PR and merge this. I hope I did a great job as this is my first time exposure to open source.
// show the week day in a number : Sunday - Saturday => 0 - 6
const daysNameList = { // weeks-day
const daysNameDict = { // weeks-day
Copy link
Collaborator

@appgurueu appgurueu Oct 7, 2022

Choose a reason for hiding this comment

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

Make this an array please: Keys are 0 to 6.

Copy link
Author

@sharansukesh1003 sharansukesh1003 Oct 7, 2022

Choose a reason for hiding this comment

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

Changed it to an array.

Copy link
Author

Hi, can you please have a look at my last commit and if everything thing is correct, can you approve my PR I need these points for Hacktober 2022 :P, thanks in advance.

// return the weekDay name.
return daysNameList[weekDay]
year %= 100
const weekDay = (year + Math.floor(year / 4) + Math.floor(century / 4) - 2 * century + Math.floor((26 * (month + 1)) / 10) + day - 1) % 7
Copy link
Collaborator

@appgurueu appgurueu Oct 8, 2022

Choose a reason for hiding this comment

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

You don't need the % 7 here if you apply % 7 below.

Copy link
Author

@sharansukesh1003 sharansukesh1003 Oct 8, 2022

Choose a reason for hiding this comment

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

Hmmm understood, changed it.

@appgurueu appgurueu changed the title (削除) Fixed DateToDay.js using Zeller’s Congruence. (削除ここまで) (追記) Fix DateToDay (追記ここまで) Oct 8, 2022
Copy link
Collaborator

appgurueu commented Oct 8, 2022 •
edited
Loading

Hi, can you please have a look at my last commit and if everything thing is correct, can you approve my PR I need these points for Hacktober 2022 :P, thanks in advance.

I am torn on whether I should merge this or #1125. Initially this seemed like the better option, but the other PR has improved significantly in response to my review comments and is mostly on par with this one.

 

removed %7 from line 36
Copy link
Author

Hi, can you please have a look at my last commit and if everything thing is correct, can you approve my PR I need these points for Hacktober 2022 :P, thanks in advance.

I am torn on whether I should merge this or #1125. Initially this seemed like the better option, but the other PR has improved significantly in response to my review comments and is mostly on par with this one.

Do mine :P, just kidding. I guess, merge the one that is better in terms of performance. Honestly I got to learn a lot from just this one contribution. If there is any other (good first issue) then maybe assign it to me :).

Copy link
Author

If you decide to merge the other PR can you label mine as hacktoberfest-accepted that will give me at least 1 point 😀
raklaptudirm reacted with thumbs up emoji

Copy link
Collaborator

Closing as #1125 has been merged. Sorry. Thank you for your effort.

@appgurueu appgurueu added the hacktoberfest-accepted Accepted to be counted towards Hacktoberfest label Oct 10, 2022
Copy link
Author

Closing as #1125 has been merged. Sorry. Thank you for your effort.

No problem, I learnt a lot thankyou for all the inputs 😀

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

@appgurueu appgurueu appgurueu left review comments

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

Assignees
No one assigned
Labels
fix Fixes a bug hacktoberfest-accepted Accepted to be counted towards Hacktoberfest
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

DateToDay.js giving wrong day

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