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

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.

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

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

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.

   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.

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!

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.

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.

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.

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

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

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

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.

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.