Ultimately, it comes down to who will maintain this. If you're hoping to pass off ownership of this and let someone else maintain it, go for the longer but simpler approach.

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

Your problem here is that you're mixing logic and presentation, so it's neither clever nor clear. I'm not 100% sure what you're trying to do (your explanation again mixes everything together making it hard to understand) but I think you want to do this:

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

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;

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.

Fewer lines of clearer code: I reject the dichotomy.

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

I am totally in the simple, but longer camp. There's absolutely no benefit from being overly clever about your implementations. It's just going to take you 10x longer down the road to figure out how to debug the damn thing.

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.

I wrote this function that converts positions on the calendar (left and right CSS offsets) to UNIX timestamps (and another function that does vice-versa).

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.

You should have more abstraction. Date handling code is rarely clear. Embedding it presentation code is rarely advisable. Calculate day of week in one function. Calculate time of day in another function then generate position based on these functions.

Won't this completely break down in a DOM simulator?

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!

It is sometimes reasonable to "compress" released code, especially if it can be done automatically and there is benefit to doing so. In the case of JavaScript, the main benefit is that the file download size is smaller.

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.

put back the intermediate variables. just because you don't name them doesn't mean the compiler doesn't have to store temporary results. you might as well give them names so the next person can read this.

In general, if I need to maintain someone else's code, what I see the most important is that the author's intent is expressed clearly. Whether it is clever-and-compressed code or simple-and-verbose code seems a secondary issue. Longer code that has too much details (such as too many names for intermediate variables) could obscure the intent of the author, and I'd rather prefer clever code to it as far as it's intent is clear, for I can make it black-box in my mind while I'm reading the whole and only analyze it lazily as needed.

I won't comment on the design concerns. Many people have done that and I could swing either way on your choice of implementation strategies. Instead, I'd like to comment solely on the code.

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 should favor simple, elegant and flexible code. Why? Because it tends to be the code that is the easiest to prove correct. Even if you don't want to prove it correct, that kind of code tends to be easy to read, easy to adapt and easy to change.

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.

What you have there is a gen-you-wine clusterfsck, my friend.

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.

It's worth noting that in some parts of the world (including the US) there are days which are 23 hours long and others which are 25 hours long, which occur when daylight savings time starts and stops. (I'm guessing your calendar isn't high resolution enough to worry about leap seconds.) So if I'm reading your code aright, the timestamp will be off by an hour on two Sundays a year in places where DST is observed.

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.

1. With better formatting it would be more readable. It's hard keeping track of the parens and braces.

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.

If you find yourself wondering if your code is sufficiently clear or appropriately maintainable, then you already know the answer.

When there is doubt, there is no doubt.

undefined

undefined

I prefer to write slightly longer, clearer code.

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

In general, I prefer more clever code that gets documented extensively. Sometimes you can get too clever and introduce funky bugs.

(clever code is faster)?clever:clearer...wait, what?