Skip to main content
Code Review

Return to Answer

Moved <hr>
Source Link
200_success
  • 145.6k
  • 22
  • 190
  • 479


Your reverse() is inefficient in three ways. By appending one character at a time to the array, the machine is unable to tell how large the array should be, and therefore may need to silently reallocate the array if it turns out that it needs to be longer than it guessed. By appending one character at a time to the resulting string, you make it take O(n2) time, since each append operation results in a new string, necessitating the entire intermediate string to be copied.


Your reverse() is inefficient in three ways. By appending one character at a time to the array, the machine is unable to tell how large the array should be, and therefore may need to silently reallocate the array if it turns out that it needs to be longer than it guessed. By appending one character at a time to the resulting string, you make it take O(n2) time, since each append operation results in a new string, necessitating the entire intermediate string to be copied.


Your reverse() is inefficient. By appending one character at a time to the array, the machine is unable to tell how large the array should be, and therefore may need to silently reallocate the array if it turns out that it needs to be longer than it guessed. By appending one character at a time to the resulting string, you make it take O(n2) time, since each append operation results in a new string, necessitating the entire intermediate string to be copied.

Source Link
200_success
  • 145.6k
  • 22
  • 190
  • 479

translate() is best done using regular expression substitution. (It's unclear from the problem specification how uppercase inputs should be handled.)

function translate(str) {
 return str.replace(/[^aeiou]/gi, '1ドルo1ドル');
}

Your sum() and multiply() are fine, except that you forgot to declare var i, so it's global. As a nitpick, I'd rename totalproduct in multiply(). You could write them more succinctly as


function sum(array) {
 return array.reduce(function(n, total) { return total + n; });
}
function multiply(array) {
 return array.reduce(function(n, product) { return product * n; });
}

However, that's not necessarily better than your original code. For one thing, the one-liner version is about ×ばつ slower.


Your reverse() is inefficient in three ways. By appending one character at a time to the array, the machine is unable to tell how large the array should be, and therefore may need to silently reallocate the array if it turns out that it needs to be longer than it guessed. By appending one character at a time to the resulting string, you make it take O(n2) time, since each append operation results in a new string, necessitating the entire intermediate string to be copied.

This should be faster. The one-liner by @elclanrs would also be quite good.

function reverse(string) {
 var rev = new Array(string.length);
 for (var i = string.length - 1; i >= 0; i--) {
 rev[i] = string[string.length - i - 1];
 }
 return rev.join('');
}

findLongestWord() is misnamed, in my opinion. You want the length of the longest word, not the word itself.

Again, you forgot to declare var i.

I'd prefer a higher-level functional approach, as Math.max() and word.length provide better hints at the purpose of the code, and therefore make it more readable in my opinion. However, your approach isn't bad. Note that my version returns -Infinity for an empty array; if you want 0 then you would have to insert a conditional.

function findLongestWordLength(array) {
 return Math.max.apply(null, array.map(function(word) {
 return word.length;
 }));
}

Your charFreq() is fine, but I'd tweak it to be more concise.

function charFreq(string) {
 var freq = {};
 for (var i = string.length - 1; i >= 0; i--) {
 freq[string[i]] = 1 + (freq[string[i]] || 0);
 }
 return freq;
}
default

AltStyle によって変換されたページ (->オリジナル) /