Ask YC: Fewer lines of clever code or more lines of clearer code?
22 comments
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."
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."
I don't think it really matters who is going to maintain the code in the future. Even if you think you're going to be the sole person maintaining the code for a long time, why is being "as clever as you can" a win? 5 years down the line in a large system, you might not remember the reasoning behind your earlier cleverness anyway.
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.
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.
Kernighan is in error for those cases where in the process of coming up with a clever solution you make yourself smarter.
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.
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.
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.
- 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.
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.
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.
A separation of concerns is not just good for flexibility, it also helps with debuggability and readability and is a reflection of ability.
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:
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.
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.
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:
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).
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).
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.
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.
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.
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.
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.
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.
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.
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.
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; })
);
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 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!
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 :).
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.
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.
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.
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.
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.
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.
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.
But mashing everything together into a big lump just makes for a exhausting debugging session in six months.
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).
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.
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.
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.
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.
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.
When there is doubt, there is no doubt.
[deleted]
[deleted]
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.
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.
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.
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)
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?
EDIT: Am I writing Lisp in Javascript?