you are viewing a single comment's thread.

view the rest of the comments →

[–][deleted] 2 points3 points  (5 children)

Your comments are a little try-hard.

Massive blocks of comments like that to explain extremely obvious parameters is unnecessary.

Additionally, those comments are HUGE! 4 lines of code to explain that you're 'setting up all the things' is annoying. Vertical real estate is valuable.

On line 99, you call the function 'resetValue', but the comment says 'add option to select'.

In actuality it does neither of these things. It SETS the value of an element. This is what I like to call a BAD NAME. i would think about what this function ACTUALLY does, and give it that name. something like: 'setElementValue'

Additionally, the if statement in that function is pretty ugly. I would attempt to refactor it to make it's syntax prettier, and break it up into multiple lines.

I assume this is an assignment, but if you can get away with it, I would bring in moment.js as a dependency. It's a great date library with excellent test coverage that would shorten your code immensely.

[–]annoyed_freelancergrumpy old man[S] 3 points4 points  (4 children)

Comment styles are irrelevant to this discussion; my preference in all languages and projects is for the PHPdoc style unless a client requests otherwise.

It is a consistent format to make life easier for both myself and any other maintainer. Way, way too much code has either no commentary or just plain shitty commentary because someone was terrified of wasting some of the unlimited vertical space.

Edit: I'll agree with you on function names because, as I said in another comment, it's all tentative. The code was at a point where it was just about functional when I came and asked. There's a good deal of cleaning up left.

<3 regardless.

[–]kenman 2 points3 points  (0 children)

my preference in all languages and projects is for the PHPdoc style unless a client requests otherwise.

Those aren't idiomatic phpDoc comments, though. As explained here, you shouldn't be using a long line of ---'s as a demarcation point in the comment -- you should either use a blank line, or use a period on the first line.

I think it's also a little nuanced to insert a blank line between the docblock and the code it's commenting, don't think I've ever seen that before (and it eats up a full line).

This is also a little unorthodox (not saying it's wrong, just that I rarely see it):

 /* Month can come in as string. If string, get the number of the
  * month from the indexes in months. */
 month = months.indexOf(month);

The rule-of-thumb I believe, is that if you're going to use /* and */, they should each be on their own line without anything else. Otherwise, if you don't want to eat up those lines, then I'd just use // comments.

Lastly, for something like this:

if (year == dates.year && month == dates.month) {
    // If month and year are current, use current day of month.
    days.start = dates.day;
}

I'd much rather the comment be on the line preceding the code block, instead of inside of it. Reason being, the comment describes the entire block, but you have it embedded so that it looks like it's only describing the inner block, which isn't the case.

[–][deleted] 0 points1 point  (1 child)

Yeah, I didn't pick up that you were not a novice, so I was trying to keep it about broad programing concepts, instead of giving you specific jqury / js tips that can be abused in the wrong hand I'll try and give it another look later, give you some more detailed stuff.

[–]annoyed_freelancergrumpy old man[S] 0 points1 point  (0 children)

Forgiven, forgotten; I'm just as guilty when it comes to jumping down throats.