Ask YC: Fewer lines of clever code or more lines of clearer code?

Re:Ask YC: Fewer lines of clever code or more lines of clear... by nostrademons on Sunday June 22, 2008 @05:39PM (#224442)

It's confusing for me...I had to puzzle it out to understand it, and even then don't fully understand it. If I were writing this from scratch, I'd skip all the pixel computations and instead add "day" and "halfHour" attributes onto the elements themselves as you're building the display (or just attach a UNIX time directly, as neilk suggests). Then computing the UNIX time is just:

   this.parent.timeStart.getTime() + 
     ((parseInt(el.attr('day'), 10) * 48) +
      (parseInt(el.attr('halfHour'), 10)) * 30 * 60 * 1000;

A couple readability/bugfixing comments on the exising code:

1.) In C-family languages, I really don't like functions that have the closing brace on the same line. I've tried it in my own code a couple times and always had to go back and change it when I went to reread the code. I have no problem with it in Lisp, but it's so different from normal coding C-like coding conventions that it makes skimming the code very difficult.

2.) parseInt automatically skips everything after the first non-numeric character, so you don't need the substr in px2num.

3.) You should get in the habit of supplying a radix argument to parseInt (i.e. parseInt(el.css(attr), 10)), because otherwise it'll interpret it in octal if the string starts with 0, which can lead to very strange bugs.

4.) When you have a known number of arguments, use function.call(obj, arg1, arg2) instead of function.apply(obj, [arg1, arg2]).

5.) In this case, it's probably clearer if you just used an accumulator; using a closure doesn't buy you anything, and means you need to mess with the this parameter.

[ Parent | on HN ]

Re:Ask YC: Fewer lines of clever code or more lines of clear... by technoguyrob on Sunday June 22, 2008 @06:10PM (#224452)

The pixel computations are done because of the way this is used in the calendar. When a user resizes the event, some kind of pixel computation is going to happen at some point, since mouse coordinates have to be transferred in some way to "left" and "top" offsets (perhaps by first translating to the start time and end time abstractions). This was probably the dirtiest part in that respect, as it is the point at which I chose to translate between pixels into usable data. It's a helper function, so to speak, so I don't have to deal with any of this in my more general abstract code (which does indeed follow the much more "clean" approach). Hence the underline before the method's name. :) (this is part of an object)

1. I agree, but they're tiny inline functions so that's why I did that. I'll refrain from doing that from now on as this seems to be a general consensus.

2&3. Aah, good point. I just went back and looked at an old JS project and noticed I was doing both of these properly with parseInt. I've forgotten though as I haven't done too much heavy Javascript for a while before this. Thanks so much!

4. I've used call before like that but for some reason I forgot about it and switched to apply. Thanks!

5. Ok, that's what I wondered. The comments so far seem to agree this would be better for readability. I don't plan on anyone else even touching this code, so indeed like someone else suggested that could be an important factor, as indeed if this wasn't a personal project I would've used a more orthodox (i.e., less compressed) approach.

That's exactly what I was looking for. Thanks!

EDIT: Also, if I used "day" and "halfHour" attributes they would be hardcoded. Maybe someday I'd like to change to hour blocks, or have a view in four-hour blocks instead of half-hour. I am using this object as an abstract class that handles a calendar view, which will be passed specifics for the view. For example, the day view will be initialized like this:

			 arguments.callee.$.__init.call(this,
				{timeblock:1800, rows:48, cols:1, timedir:0,
				startTime:startTime,
				timename:'day', container:'daycontainer'}
			);

Where the "arguments.callee" stuff is just calling the (inherited) superclass initialization function. This way, if a New World Order appears and ever decides to make 5-day instead of 7-day weeks, all I have to do is change one line of code. ;) (while that's true, the real reason for me doing this is so I don't have to write code 3 times for a day, week, and month view)

That's (partially) why I didn't do something like hardcode "day" or "halfHour" attributes (although I do store all this information in the object--the method I showed above is a computational helper function).

[ Parent | on HN ]