bchurchill
commented
7 years ago Things to fix before merging:
-
There need to be clear instructions on how to generate a distributable badi.js file for browsers. There should always be a folder with a final javascript file in the repository so someone can, if they like, download a version of the library that's compatible with all major browsers. They shouldn't have to download or install any tools, and they shouldn't have to look hard. It would be preferable (but it's not necessary) if there were a human-readable file for this purpose (the packaged code is a real pain to read).
-
Right now, it's possible to merge the master branch into the gp-pages branch so that github can host the example. It's okay if that particular mechanism no longer works, but we need an automatic way to publish to github pages. One option would be to host all the website pages out of the /docs folder and provide an easy way to repopulate that folder with a newly-built badi.js.
-
Function/variable names need to represent what functions/variables do. For the most part it's okay, but some names changed in ways that obfuscate behavior. For example,
find_sunsetis now calledgetGregorianDateForSunset. What this function actually does is it calculates the time of sunset on a given gregorian day and returns that in a date object. However, the word "For" in the function name implies that you give the function a "Sunset" and you get a GregorianDate back, which doesn't make sense. A better name would begetUTCTimeForSunsetOnDateor maybegetSunsetTimeOnGregorianDate.
Frequently, the name "Gregorian" is misused, and it would be more
meaningful to write "UTC". UTC corresponds to universal time without
regard for a system of calendar (except that internally its stored
in a gregorian way). Some of these "Gregorian" variables really
represent a moment that's significant on the Badi calendar and not
the Gregorian one. Also, keeping track of which dates are in UTC and
which are local to a place is often the real chore, whereas keeping
track of whether something is stored in a "Date" object isn't. For
example the gregorianSunset and gregorian8NewMoons variables in
getGregorianDateForTwinBirthdays. There's really nothing "Gregorian"
about these value other than the fact that it's stored in a date
object. However, it represents a specific UTC time. It would be more
sensible to call them UTCSunset or UTCNewMoons. With all the name
changes, understanding what the function does now is harder.
-
Helpful comments that were previously in the code are missing. For example,
find_naw_ruzused to have the comment "Gets the day of Naw Ruz in a given gregorian year. It returns the day at 00:00:00 UTC". I haven't looked for other missing helpful comments, but if there are any, they should be replaced (I just came across this because it was relevant to one of the questions). -
Conditionals.
The syntax (conditional ? a : b) is awful to read. An if-statement
is almost always preferable. It's not worth using this syntax just
to make a variable const -- let it be mutable!
There is no advantage in using this conditional notation on a statement that would take up multiple lines anyway. An if-statement is always more clear than this.
Even in the most extreme cases where use could be justified, like
x = function(foo, bar, oof, doof, goof, ( awesome ? 3 : 4 ))`I prefer (although this is just me),
if(awesome)
hoof = 3
else
hoof = 4
function(foo, bar, oof, doof, goof, hoof)-
Remove comments with "QUESTION". I've answered them below.
-
There are several yarn.lock files in the branch which I assume shouldn't be there (this is for maintaining local machine state, right? I could be misunderstanding).
Things I would like to see fixed, but that I won't insist on:
-
Many functions are in their own file. This just creates more work for to edit the project. Relevant functions should be grouped together. At least
getGregorianDateFor{NawRuz,NextNewMoon,TwinBirthdays}could be together. -
I don't like camelCase. If possible, I prefer underscored_names unless there's a really good reason to change it. At least just CapitalizeEveryWord.
-
I prefer that file names don't start with underscores unless there's a technical reason why it should be that way.
-
There are 544 folders in
node_packagesin the source tree. Do these need to be there? If so, why? Do users need these packages or just developers? -
It's sometimes not worth wrapping a line of the form
let variable = function(foo)into two lines. If the line is just barely too long, the cleanest solution may be to make the variable or function names shorter. This is the case, for example, with the line that declaresgregorian8NewMoonsin_getGregorianDateForTwinBirthdays. It might also be okay to wrap lines a little longer. I'm guessing right now it goes to 80, but I'm okay with it as high as 100 (it'll still fit on 1/2 of my screen). Middle-ground like 90 is fine too. But that's up to you. -
In general I'm a little disapointed with the added complexity. This library was designed to be a one-file drop in that shouldn't be hard to use, hard to develop or hard to setup. Now, anyone who knows only javascript could work on the whole project. With these changes you also need to have some level of familiarity with node, webpack, jest, and babel to work on the project end-to-end (and probably a CI tool, coming soon). If there's anything we can do to reduce the number of tools I would appreciate it.
Along these lines, I would appreciate a reminder on why this packaging is helpful. I don't see improvements to ease of development or library use. Does the packaging really make it easier for developers to use the library on other platforms? If so, how do they do this? Where do they find instructions?
Questions:
fromGregorianDate: We are getting the UTC year here, is that what we want?
Yep. We could already be a whole year off anyway depending on when Naw-Ruz is, and so the if-statement a few lines down corrects this. Getting the "localtime" year would be very involved because it would require tracking down the timezone of the given latitude/longitude coordinates.
fromGregorianDate: Why are we subtracting two days?
- We start with the date of Naw Ruz at 00:00:00 UTC
- Then for every Badi month that's passed we add 19 days
- We also add one day for each day on the Badi calendar. HOWEVER, we've already added one too many because the first "day" of a month corresponds to not incrementing the gregorian date at all. In other words, the Badi days of the month are 1-indexed, but the number of days that we need to add to the starting date should be 0-indexed.
- Now, the date we currently have is at 00:00 UTC. This means that the Badi day actually started at sunset the night before. So we need to substract one more day to get the right sunset.
getGregorianDateForTwinBirthdays: Why are we getting sunset for naw
ruz date? Is it not already sunset.
From the comments in the original source code of find_naw_ruz:
"Gets the day of Naw Ruz in a given gregorian year. It returns the
day at 00:00:00 UTC." Thus, you need to get the sunset time in
Tehran on this day to find the moment the year starts in Tehran.
There's a good reason for this. The year starts at a different time
for every place in the world. It starts at sunset, Tehran time, in
Tehran. But it will start earlier in India and China (when the sun
sets there) and later in Europe and the Americas. Forcing the time
to be at 00:00:00 makes it clear to the consumer of this value that
they need to really think about what it means to have a Gregorian
day which "corresponds" to Naw Ruz.
Regarding the questions about getGregorianDateForNextNewMoon:
The BlueYonder function MoonQuarters returns the day/time of the new moon, first quarter, full moon, and third quarter in an array of length 4 (Julian dates). jdtocd is a function that converts Julian dates to Gregorian ones (I didn't name that function...) The problem is that it might return the wrong lunar cycle and give a new moon date prior to the date we're looking for. So a second call to the MoonQuarters function needs to be made -- this time with a date that's squarely in the next lunar cycle. This function could be written without recursion, but it would essentially require duplicating the code that's already there or writing a loop. I think the recursive one is nicer, although admittedly this code is tricky and could use a few comments.
getGregorianDateForTwinBirthdays: Why are we setting to sunset in the
previous line if we are just getting the day in the next line?
I'm not totally sure. It's possible that this was intended to compensate between the time difference of Tehran and UTC, but I don't think it's actually necessary. Another possibility is that an older version of the code was returning the time, and then it got changed to the current interface without removing the dead code. If all the tests pass with this statement removed we can take it out.
brenmcnamara
I added a command
npm run create-buildthat will build the javascript and deposit it into "browser-build-v1", "browser-build-v2", and "build-v2" directories. The "browser-build" directories can be imported straight into a browser the way you would normally do it and "build-v2" is not bundled, assuming that you are working in an environment that supports some module system. I made of note of this in the README file.I understand that the packaged code is unreadable, and I feel that it should be left that way. If someone wants to read the code, they should be looking at the source code. People shouldn't be reading the packaged code, analogous to any other language where you wouldn't expect people to read compiled binaries. In the future, I would like to add things like an "uglification" step, to compress the javascript and make the library more efficient, making it even more unreadable.
That's definitely an awesome idea to have documentation and a built javascript file continuously pushed. I would like to save the work for after the pull request, if that's okay. I feel that this is a significant project on its own.
I renamed the variables / functions by your recommendations. When coding, I prefer that the data type of a variable or return value of a function be clear in the naming. I've ran into a lot of trouble in the past, especially with date objects, where people give un-descriptive names like "timestamp" or "date" without any clear indication of the format (timestamp in seconds / milliseconds? Date according to what standard and format?). This is why I tend to name my variables with names that express the class type in which the date is being storing "gregorian is javascript Date, badi is BadiDate". I understand your argument for the name "UTC" over "gregorian" for relevant cases, and that sounds reasonable to me. I made the changes. I apologize in advance if I missed any renaming of dates. I see you also added a recommended naming scheme in a later comment. It sounds like a great idea. I would like to delegate that work to you if that's okay, you have a better understanding of the semantic differences between gregorian and utc dates than I do.
I went through the original code in master and migrated any documentation that I found missing. Again, I apologize in advance if I missed a spot.
I went through and replaced all ternary uses with if / else. Personally, I prefer the ternary operator for a couple of reasons. First, it's more succinct. I've seen it used enough in code, that I read it as comfortably as any if / else statement. Second, I prefer having my variables assigned in one statement on avoid mutability. The "let" keyword indicates an intent to re-assign a variable. In a long block of code, reading a "let" puts in my mind that the variable will be reassigned later, and when the re-assignment is done only because of a conditional initialization, I still have to make sure that I understand the value of the variable throughout the function. Const variables give me assurance that the variable is consistent throughout the function.
With all that being said, I don't feel strongly enough about this to push back, as long as the style is consistent, I'm fine with either approach.
Remove QUESTION from files
On the topic of yarn.lock
The yarn documentation specifies that yarn.lock should be added to source control. Here is the relevant documentation. Check the section titled "Check Into Source Control"
I moved the 4 main functions you list into a file called _util. If you have a better name for that file, I'm up for suggestions (I don't like naming things "util"). I would like the holy days and the geo-location information in separate files. The reason is that they are data, not code, and I don't like to mix code and data.
The reason I moved things to camel case is because that is the known convention for javascript. All the public API's and any third-party libraries follow this convention. Upper camel case is used for classes, lower camel case for functions, methods and variables. Constants don't have a clear convention, I've seen the following:
HERE_IS_A_CONSTANT, HereIsAConstant, hereIsAConstant. I tend to straddle between the first two conventions, but I'll let you decide on it. There are not many common conventions in javascript code, but camel case is one that is consistently seen. If we are introducing a public library, we should stick with whatever conventions the developer community expects.Changing underscore files to not contain underscore. In general, however, I use the underscore convention to denote private. Private variables / methods / files are not enforced in javascript, so this is the naming convention I like to indicate that I don't want people to touch my file / method. In Facebook, underscore methods are enforced by walking through a class and renaming all the methods that start with underscore, so that any caller outside of the class will break if trying to access the method. I don't feel strongly about having underscore filenames, but I would like to stick to the convention of underscore methods == private.
I am adjusting the eslint configuration to enforce 100 columns instead of 80. I typically stick to 80 so that I can have multiple side-to-side panes of code and read comfortably, but I don't feel strongly about this. Many of the variable names become short when I converted gregorian => UTC. In a later comment, you made the suggestion of using "greg", which I'm fine with also.
I understand your concern about the increased complexity of this project. babel, webpack, node, and yarn is a lot more tools than a simple javascript file that can be added to a project. This added complexity is the cost of having a javascript implementation that is portable and accessible. The hope is that with good documentation, pre-made builds, and possibly some CDN deploy that contains the built javascript for people to reference, the need for in-depth knowledge from the user is not needed. Oliver needs a version of this that works with his app that he developed, but the original version of this project is not usable in his javascript environment, so the 2 options available are to fork this project or to modify it to support all environments. If the complexity is something that is a deal-breaker for you, I can fork this so I have an implementation for Oli and we don't need to introduce this complexity to the original. If it's not a deal-breaker and you're willing to move forward, we can work together to find a way to minimize the overhead for you and other developers.
The packaging might seem daunting right now since you just spent a bunch of time downloading all the tools, setting up your environment, and reading through the internals. However, once this module is published to npm, it's very simple to use:
From the command line:
npm install --save badi-calIn your code:
The package manager give the following to use for free:
In order to make things easier web developers with a simpler setup who just want a js file to drop into their project, we can deploy to a CDN, like surge, and provide a public link for them to access, then include that link in the documentation. (And we also provide builds for the browser within the repo for conventience as well).
npm run testfailingThere was a small bug I forgot to fix before pushing. It is fixed now and the tests pass. Sorry about that.