If it's something that you'll be in charge of forever, get as clever as you can :)

Remember the words of Kernighan: "Debugging is twice as hard as writing the code in the first place. Therefore, if you write the code as cleverly as possible, you are, by definition, not smart enough to debug it."

The complexity in a program should come from the problem it is attempting to solve, not the trickiness of the implementation. I personally prefer code that is as simple as possible (but no simpler, to coin a phrase) -- there are enough things that can go wrong in a complex software system without adding overly-clever code into the picture.

I think one of the primary guiding principles of software design ought to be simplicity: strive to make everything as simple as possible, except where other requirements like efficiency, robustness, functionality, or portability supersede.

And smartness can be highly conditional. Clever code is often written by calm and well-rested people, but its showstopping bugs are often fixed at 3am by panicked people who are losing thousands of dollars per second.

- Transform the screen position to a week-based relative coordinate system.

- Convert that spatial coordinate into a time stamp represented as seconds since the epoch.

That's a minimum of two steps, two verbs and therefore two separate functions. Step 1 can be broken down further by introducing helper functions for inspecting the DOM, decoupling the representation of the UI elements from your actual screen-space coordinate system, as CSS isn't in a nicely-scriptable format.

Once you've done this, you'll discover the result to be only marginally longer, but highly reusable.

EDIT: And yes, I agree, if I ever decide to make events more fancy than just dropping in a DIV on the timetable, I'll probably have to do a bit more. But of course, only a couple things will change then, and I can just write those as helper functions for this function.

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

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.

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'}
			);

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).

So first you have four lines of comment explaining a completely unnecessary helper function--px2num(el,'top') doesn't save a lot over parseInt(el.css('top')), and the later is clearer. Then you have all packed into your return something that's technically one line but actually takes seven lines to display in a way that it can actually be read.

So could this be written clearer in less than ~12 lines? Oh yes. First, why are we working in seconds when JavaScript uses milliseconds natively? Assuming that's really necessary, you must do myDate.getTime()/1000 an awful lot. So let's assume you added a getTimestamp method to Date.prototype. And I'm going to assume that el.style.left actually has to equal one of the members of hSpacing--it doesn't make sense that you'd allow dropping without snapping to the grid and that you'd only want the right result if they happened to drop left of the day column but not if they drop one px right.

And it looks like you've got jQuery mapped to j instead of $, which is weird but ok.

    function domStartTime(el) {
      var week = this.parent;
      return week.timeStart.getTimestamp() +
        week.timeslice * j.inArray(parseInt(el.css('left')),this.hSpacing) +
        (parseInt(el.css('top')) + this.borderWidth) / week.rows / this.vSpacing;
    }

    function domEndTime(el) {
      return domStartTime(el) +
        parseInt(el.css('height')) / this.parent.rows / this.vSpacing;
    }

My first programming job was a shop where we actually wrote reference code that would be implemented by other companies into various chipsets. There was no room for lack of clarity there. Everything, absolutely everything, must be crystal clear. I've taken many of the things I've learned there to all my other programming projects.

This seems like a very bad design. Is it really better than simply attaching a time property to every div at calendar creation? You are coupling presentation and data, which is in general a terrible idea.

Also, you realize that other countries arrange days of the week differently? If you ever internationalize, you will be in a world of pain. If you ever decide you want column width to vary for some reason, again you are fucked.

You can sometimes justify tricky Javascript since download speed is a factor. However, your JS strikes me as probably more wordy and complex than the straightforward solution.

Tricky techniques that exploit tight coupling are almost always wrong.

Does that make my approach any better? I think that addresses every issue. I agree, though, there are better ways to do this, but this isn't a priority project for me so I haven't put as much thought into it as I'd like. Thanks, Neal.

To borrow MVC terminology, your DOM element is then effectively a controller + view in one. Just like we might translate a slider at 50% into 128/256, it's okay to take its properties and then translate those into your model. Presumably you poll the DOM object as it is being dragged, or fire off events when it stops being dragged, which updates the model in near-real-time. Do I understand it?

This is actually a good idea then. Although you want to be careful that your controller doesn't rely on magic constants to do its conversion, it should derive those from some initialization from something representing a "layout". But you seem to have done this.

To get back to your original question... I did find the code hard to read. I don't have the time to unravel it to make it clearer, but personally, I always like to state the problem as clearly as I can up front, in an expression that approaches pseudocode. Like "return getStartOfWeekTime(el) + getOffsetTime(el)". This hides away the hard bits in smaller routines, and the clueless maintenance programmer (i.e. you in one month) will understand what's going on right away and where the bugs might be. But if this is too slow you may have to bite the bullet and live with something hard to read.

		this.hSpacing = Array.prototype.slice.call(this.parent.blocks.filter(
				function(n){ return n < _this.parent.cols; })
				.map(function(n,o){ return o.offsetLeft; })
		);

However, unless there is an absolutely measurable and significant advantage, I find "clever" code to be far more trouble than it is worth. Engineer time is very expensive, and you don't want people sitting and studying code for very long just to figure out how to change it safely.

It seems like a bad idea to couple data with presentation.

I'm quite honestly dismayed when my team try stupid things like this. I can give a recent example, of where we perform stats from data stored in a grid, where performance of the grid is terrible, and it drags the stats performance into the ground, instead of quickly calculating/caching the stats from the data directly.

If you suggest adding data caching functionality to a GUI grid, I will start crying!

1. Use a table, or horizontal grid of divs to represent the calendar. You can write different server or client side code to render different presentations, e.g. calendar, 5 day view, 7 day view etc.

2. In this grid, tag each div with the appropriate date stamp as appropriate for your application.

3. Build code on the client side to track mouse drags, clicks, or whatever you need to provide the user with a great experience specifying start and end times.

None of this sounds that hard or tricky, and I would certainly be happier with an approach that separated out generating time tagged divs from the presentation and selection mechanism.

EDIT: I see what you mean. For part 3, for example, I want "12:05", not "12:04:27". Yeah, I was getting to that.

But mashing everything together into a big lump just makes for a exhausting debugging session in six months.

My preferred measurement is to look at how easy it would be to prove a couple of nontrivial theorems about the code you are looking at. The easier that would be, in a perceived view, the better I find the code. If you kludged 2-3 things of no relation into the same function, it proves to be extremely hard to state meaningful cases in proofs about it.

Good code arises with 2 things: Persistence and iteration. It is when you leave code and never come back to improve it that things begin to go wrong. Think about it: All successful source code projects have these two traits. People keep coming back to read old code and improve it. They persistently iterate the code into better shape. Hence, you should favor code that is easy to read and understand. If it is not, then it will be rewritten in the next iteration when someone glances at it. Unless it contains some elegant piece that saves you a lot of the headache -- you will know when you try to rewrite it into "simpler" code. The persistence and iteration of old code to better it, is what most bad management do not understand about software construction.

As an agilist, I'd say: "The code works, so it adds business value, and therefore the rest doesn't matter." However, I am not solely an agilist. I believe this code is too hard to maintain.

This code is terse, which is often good. However, it is also too clever for me to follow in a few minutes. It's a library function, so there isn't much to gain from being terse or clever.

You could do one of several things, but here are the two I'd consider:

1. Keep the logic the way it is, and attempt to use formatting and commenting to make it easier to follow. Use more vertical lines and maybe use sidebar commenting to make each line clear in its intent.

2. Follow the advice of the folks who say that you're not really saving compiler time and unroll the logic into more definitive chunks that use variables to illustrate what you're up to.

In this case, I prefer #2, because I suspect you may have put more effort into writing fewer lines than you may have been able to put into a solution that used more lines. If you consider the cost-per-line-of-code, you may not have saved anything.

That said, congratulations. You may want to consider entering an obfuscated code content. :) (Just kidding).

You need at least a handful of variables to name and hold your intermediate values. Putting names on things is good. It lets you refactor and debug your code in a reasonable way, without doing partial function application in your head.

Furthermore, it aids in doing reasonable functional decomposition. If you have several terms to which you're applying similar operations, that may be a natural target for abstraction via a function. Without the intermediate assignments, though, the syntactic noise of parenthesis, ternary statements, and weird spacing tends to mask the commonalities in actual code flow.

Even worse, by making so many inline function definitions and applications, you've re-defined 'this' to mean several different things in a single scope. Full-time JS jockies may be used to that, but it makes my head hurt.

Code like this screams to me that the author would rather rewrite it completely (or force someone else to do so) than change it.

But to answer your original question, I'd have to say it depends. If the clever stuff is really much more efficient, or less likely to break in unusual circumstances, I say use it, but document it carefully. Otherwise, best to code it clearly.

2. What's with the call to parseInt? Are you trying to floor it? If so, just use Math.floor()

3. If you know the number of parameters to a function call you can use method.call(this, arg1) instead of method.apply(this, [arg1]). Slightly more readable, IMO.

Overall I would say this is a little too clever. I'd use a few extra lines to make it more clear.

When there is doubt, there is no doubt.

What I find infuriating is writing the same code all over again, with little variations over many files.