0
\$\begingroup\$

I'm currently populating before and after arrays with indexes, based on a number provided. If the input going in is an int, there will be three indexes (left, middle and right). If it is a decimal, there will just be two (left, right). There will me a maximum of three indexes to sort into a maximum of two arrays (before and after). To understand, here is some expected input/output:

input: 1
output: before [] after [0, 1, 2]

input: 1.5
output: before [] after [1, 2]

input: -3.4
output: before [3, 4] after []

input: 0
output: before [1] after [0, 1]

Essentially, the input index gets floored and ceiling'd (or, in the case of a whole number, their next/previous integers are used along with the original input). If any of the resulting integers are below 0, they get put in the before array, but their index is made absolute, otherwise they go into the after array. If they do get put into the before array, then they are put in to reverse order, such that they remain sorted in numerical order.

I have the below working code. But I feel like I could do much better. How would the community go about optimising this?

var input = document.getElementById('input');
var button = document.getElementById('button');
var ouput = document.getElementById('output');

function getOutput(input) {

  var left = Math.floor(input);
  var right = Math.ceil(input);
  var middle;

  var before = [];
  var after = [];

  if (right == left)
    left--,
    middle = left + 1,
    right = left + 2;

  if (left < 0)
    before.unshift(Math.abs(left));
  else
    after.push(left);

  if (middle < 0)
    before.unshift(Math.abs(middle));
  else if (typeof middle == 'number')
    after.push(middle);

  if (right < 0)
    before.unshift(Math.abs(right));
  else
    after.push(right);

  return {before: before, after: after};
  
}

function buttonPressed () {
  
  var i = parseFloat(input.value);
  var msg = 'not a number';
  
  if (!isNaN(i)) {
    
    var o = getOutput(i);
    
    msg = 'before [' + o.before.toString() + '] ahead [' + o.after.toString() + ']';
  
  }
  
  ouput.innerHTML = msg;
  
}
<input id="input" type="text">
<input id="button" type="button" value="get" onclick="javascript:buttonPressed();">
<br><br>
<div id="output" style="font-family: monospace;"></div>

My interest is in the getOutput method. The other stuff is for demonstration purposes.

Update

I have adjusted the expected output and the code from the original question, after being prompted to rethink from the comments.

\$\endgroup\$
  • \$\begingroup\$ The code and the description disagree on the result for -3.4. The text says that before should be [-3, -2] but the code produces [-4, -3]. Which is the intended result? \$\endgroup\$ – Edward Feb 13 '16 at 13:06
  • \$\begingroup\$ @Edward apologies, the intended result is [-4, -3]. Have corrected the Question. \$\endgroup\$ – shennan Feb 13 '16 at 13:08
  • \$\begingroup\$ And what is the task you are solving with these before/after? \$\endgroup\$ – Roman Susi Feb 13 '16 at 13:10
  • \$\begingroup\$ @RomanSusi I'm essentially trying to resolve a situation whereby I can logically reference arrays with negative indexes. In a perfect world, arrays would be able to be addressable by the index -n, but doing this is not possible with JavaScript. Not if you want to use the Array prototype properly. Perhaps there is a better way to do this, I'm all ears. :-) \$\endgroup\$ – shennan Feb 13 '16 at 13:12
  • \$\begingroup\$ Just add array length to the negative index? (not sure about values like 0.5 though) \$\endgroup\$ – Roman Susi Feb 13 '16 at 13:16
1
\$\begingroup\$

The code in the question seems to have adequate complexity for the getOutput function. Some small hints may be given, like:

  • do not use comma-expressions and decrement,
  • initialize middle with null and check for it with !== instead of typeof,
  • also, I do not like comparing undefined middle with 0.

The current code is more or less readable.

As for optimizations, one observation is that apart from [-1, 1] range, all other inputs always use the same alternative, making unnecessary to check left, right and middle individually. (eg, if right < 0, then so are middle and lift). How much optimization it really brings is hard to tell. If input numbers are almost always large, then making separate branch and constructing an array directly [left, middle, right] may make the code more efficient.

The near-zero case may need more conditions, of course (or just some kind of lookup for ready arrays - as the number of cases is small).

If you want a more compact code, maybe something like this can be done:

 tmparray = ((right == left) ? [left-1, left, left+1] : [left, left+1]);

And after that push/unshift in a loop for each tmparray element. Not sure this will be faster though, but at least may be more readable. (left may be renamed to "lower", and right calculated inline only in the condition).

\$\endgroup\$
  • \$\begingroup\$ This is a good, thorough review. Thankyou. I shall mark it later in the day when I've had time to implement some of your suggestions. \$\endgroup\$ – shennan Feb 13 '16 at 14:47
  • \$\begingroup\$ Just one point about your answer: "use of typeof where check for null may suffice". Do you mean, set the middle = null and then check? As middle may equal 0, we cannot cast as boolean with the !. Also, is typeof considered much slower than checking null? \$\endgroup\$ – shennan Feb 14 '16 at 12:15
  • \$\begingroup\$ Thanks. Clarified the wording in the answer: use null and !== null check. Explicit is better than implicit. If you will use tmparray, then middle will not be needed at all. \$\endgroup\$ – Roman Susi Feb 14 '16 at 17:55

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.