

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

This is a question mostly geared towards helping my own programming style (and of course maybe yours, by example), so if you can please reply to give me some input, that would be great.<p>I'm writing a Javascript calendar sort of like Google Calendar's functionality but with more collaboration features. For the events that actually show on the calendar, 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). In the code below, the parameter "el" is a jQuery encapsulation of the DIV that holds the event, the "isEndTime" parameter is whether to look at the beginning of the DIV or the end (i.e., start or end of the event), the this.parent.timeStart variable holds a Date that begins at the very top left of the calendar (midnight at first Sunday of the week), the this.parent.timeslice variable is a shortcut/cache for the number of seconds that one column in the calendar represents (i.e., # of seconds in a day), the this.hSpacing variable holds the CSS "left" offsets of the 7 columns as an array (60, 210, 360, etc., for a 1024-wide window resolution), this.vSpacing is the vertical # of pixels in one half-hour block (each day is partitioned into 48 rows of half-hour blocks), and this.borderWidth is just the width of the border between half-hour blocks.<p>Within a minute or two, would you be able to comprehend the following? (the whitespacing is a little ugly, but I can't go over 80 characters horizontally)<p><pre><code>   _dom2time : function(el, isEndTime) {
	// The function px2num below takes a DIV element and CSS attribute like "left", and
	// returns the number of px that CSS attribute was assigned.
	// For example, a DIV with left:50px called with attr equal to 'left'
	// would return 50.
        var px2num = function(el, attr) { return
		parseInt(j(el).css(attr).substr(0,el.css(attr).indexOf('px'))); };
			
	return
		(this.parent.timeStart.getTime() / 1000) +
		parseInt(this.parent.timeslice * (
		(function(n){for(var i in this.hSpacing) if (n &#60;= this.hSpacing[i])
			return i;}).apply(this, [px2num(el, 'left')]) +
		((px2num(el,'top') + (!isEndTime ? 0 : 
			px2num(el,'height') + this.borderWidth)) /
		(this.parent.rows*this.vSpacing))));
   }
	</code></pre>
If you can't understand it, let me just take a second to explain. The UNIX timestamp at a given position is given by the time at the start of the week, plus the days before that event, plus the seconds up until the event on the day of the event. So, I take the start-of-the-week timestamp, and then take the number of seconds in a day times the "fraction" of a day the event occurs at (it would be 5.5 for an event occuring on Friday at noon). Traditionally, this code would've been done very different by starting maybe with some variable which accumulated the time up until the event (starting with the beginning of the week, then adding the days up until, then the seconds in that day up until), but instead it's all compressed into one return statement. When I had written this, it came naturally, but when I stopped to look, I realized "Wow...this might be kind of hard to follow". And yet it's much fewer lines of code than a "traditional" approach would've used. What would you prefer?<p>EDIT: Am I writing Lisp in Javascript?
======
irrelative
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."

~~~
brlewis
Kernighan is in error for those cases where in the process of coming up with a
clever solution you make yourself smarter.

~~~
mechanical_fish
Yes, but beware: Additional smartness does not always stick.

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.

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

~~~
technoguyrob
Sorry for not being clear about that. The purpose of this helper method (hence
the underline before that name) is to take an event positioned in the DOM and
convert its position into a timestamp for that event. Probably the only place
this will be called is when some change is made to the DOM of the event. For
example, when it is resized, it'll have a different height as well as "top"
offset, so that'll have to be translated into a timestamp. I don't know any
other way to do this that would be fundamentally different, since at some
point the pixel-based resizing is going to have to become a timestamp. Nothing
in this function will be used anywhere else, hence I didn't make any more
helper functions for my helper function. Thanks!

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.

~~~
aaronblohowiak
A separation of concerns is not just good for flexibility, it also helps with
debuggability and readability and is a reflection of ability.

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

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

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

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

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

~~~
technoguyrob
Yes, I skipped over a few of the specifics. The "Saturday/Sunday" at the end
of the week versus "Sunday at the beginning" is taken care of, I was just
using that as an example to explain; internationalization works fine. The time
property is already attached to the DIV; however, this will be recomputed
based on some DOM-change to the event. It makes it simpler, since (for
example) when the user resizes the event, I can just deal with the physical
resizing of the DOM object, and once they're done, translate it to the
timestamp. I'm a presentation/data separation freak to the point of having no
Python code include any HTML, and no HTML include any JS (except the <script>
tag). However, this is a case where presentation and data are very intimately
attached. Maybe I should have explained that I don't use this to get the time
of the event. However, when the user resizes the event, at some point that
connection between data and presentation has to be made, since their resizing
is ultimately caused by pixel-based mouse coordinates. Also, the varying
column width is taken care of, as none of this is hardcoded. The vertical and
horizontal spacings are computed by looking at offets, and recomputed during
window resizing.

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.

~~~
neilk
Okay, I think I get it now. You want the user to "stretch" the DOM element
representing a schedule item, and then when the user is done stretching, you
will translate the movement into a real start and end time.

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.

~~~
technoguyrob
Exactly! And yes, all those "magic numbers" are derived. For example...

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

Your function names and decomposition are indeed much easier to read. Thanks
again, Neal!

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

------
watmough
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!

~~~
technoguyrob
This is the dirtiest component of going between DOM and the event abstraction
(start and end times). When someone resizes the event or moves it, it's going
to take some kind of pixel calculations at one point or another, even if
they're very generalized, since they used their mouse to do that. This is the
point at which I decided to deal with that. Otherwise the events are all
handled abstractly. The data isn't necessarily coupled with the presentation,
but for example the way they move the event around on the DOM when they're
dragging and dropping (presentation) has to be ___translated_ __into data at
one point or another (and so the function is called dom2time--translating
presentation to data :).

~~~
watmough
I went back and re-read your explanation above, and my constructive criticism
would be the following:

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.

~~~
technoguyrob
Yeah, I did all this. :) For part 2, I still need a helper function like this
because the timestamp changes (when the user resizes, moves, etc.). Does that
make more sense? I have a grid of DIVs, on which I position events on top of.

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.

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

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

~~~
bprater
Agreed, that's the biggest issue with clever code. I'm easily tempted to do it
too.

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

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

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

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

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

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

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

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

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

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

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

~~~
technoguyrob
Clearly clever...

