Take the 2-minute tour ×
Code Review Stack Exchange is a question and answer site for peer programmer code reviews. It's 100% free, no registration required.

Please critique my solutions to these simple exercises. I have no programming experience, so the more anal you are, the more I'll learn. I'm teaching myself, this is not for school:

Write a function translate() that will translate a text into "rövarspråket". That is, double every consonant and place an occurrence of "o" in between. For example, translate("this is fun") should return the string "tothohisos isos fofunon".

var translate = function(x) {
  var string = x.toLowerCase();
  var vowels = ["a", "e", "i", "o", "u", " "];
  var y = "";
  for (i = 0; i < string.length; i++) {
     var current = string.charAt(i); 
    if (vowels.indexOf(current) != -1)
    y = (y + (current));
   else 
    y = (y + (current + "o" + current));
  }
  return y;
}

Define a function sum() and a function multiply() that sums and multiplies (respectively) all the numbers in an array of numbers. For example, sum([1,2,3,4]) should return 10, and multiply([1,2,3,4]) should return 24.

var sum = function(array) {
  var length = array.length;
  var total = 0;
  for (i = 0; i < length; i++) {
    total += array[i];
  }
  return total;
};

var multiply = function(array) {
  var length = array.length;
  var total = 1;
  for (i = 0; i < length;  i++) {
    total *= array[i];
  }
  return total;
};

Define a function reverse() that computes the reversal of a string. For example, reverse("jag testar") should return the string "ratset gaj".

var reverse = function(string) {
  var length = string.length;
  var reversed = [];
  var joined = ("");
  for (i = length; i > 0; i--){
    reversed.push(string.charAt(i-1));
  };
  for (i = 0; i < (length) ; i++){
    joined += (reversed[i]);
  }
  return joined;
}

Write a function findLongestWord() that takes an array of words and returns the length of the longest one.

var findLongestWord = function(array) {
  var elements = array.length;
  var count = 0;
  for (i = 0; i < elements; i++) {
    if (array[i].length > count) 
      count = array[i].length;
  }
  return count;
}

Write a function filterLongWords() that takes an array of words and an integer i and returns the array of words that are longer than i.

var filterLongWords = function(array, int){
  var length = array.length;
  var longestWords = [];
  for (i = 0; i < length; i++) {
    if (array[i].length > int) {
      longestWords[longestWords.length] = array[i];
    }
  }
  return longestWords;
}

Write a function charFreq() that takes a string and builds a frequency listing of the characters contained in it. Represent the frequency listing as a JavaScript object. Try it with something like charFreq("abbabcbdbabdbdbabababcbcbab").

var charFreq = function(string){
  var list = {};
  var length = string.length;
  for (var i = 0; i < length; i++) {  
  if (string.charAt(i) in list) 
    list[string.charAt(i)] += +1;
  else 
    list[string.charAt(i)] = 1;
  }
  return list;
}

EDIT: I forgot to mention that rather than use global methods, I tried to as much as possible code for the result I wanted, like reverse, and join, etc.

share|improve this question
5  
Welcome to Code Review! I've upvoted your question (+5!), but I think you should break this down into multiple questions - not because of the length (even more than that is still perfectly fine), but because it's really multiple unrelated pieces of code. The more questions you ask, the more upvotes you'll get, the more answers will appear, more votes, more happiness, joy! [cough] .. you'll like it here ;) –  Mat's Mug May 10 at 0:08

4 Answers 4

I'd start by pointing out that you forgot to declare your loop variables, so they become global, you need var:

for (var i; ...
    --^--

Also you forgot a few semicolons after your function expressions, JSHint will tell you about these common errors.

But your code looks good, although maybe not very JavaScript-ish. Targeting ES5 compliant browsers (IE9+) you could simplify your code, so instead of using for loops, you'd use built-in higher-order functions, such as map, filter and reduce:

var sum = function(xs) {
  return xs.reduce(function(x, y){return x + y});
};

var multiply = function(xs) {
  return xs.reduce(function(x, y){return x * y});
};

var reverse = function(x) {
  return x.split('').reverse().join('');
};

var findLongestWord = function(xs) {
  return Math.max.apply(0, xs.map(function(x){return x.length}));
};

var filterLongWords = function(xs, i) {
  return xs.filter(function(x){return x.length > i});
};

var charFreq = function(x) {
  return x.split('').reduce(function(acc, x) {
    acc[x] = ++acc[x] || 1;
    return acc;
  },{});
};

For code that is very generic I prefer to use short variables names such as xs, x, y, etc. These are inspired by other functional languages, and are well understood, but you could always add JSDoc annotations, for example:

/**
 * @param {String} x A string
 * @returns {Object} Counted occurrences of characters in x
 */
var charFreq = function(x) {
  return x.split('').reduce(function(acc, x) {
    acc[x] = ++acc[x] || 1;
    return acc;
  },{});
};
share|improve this answer
1  
Thanks man! I basically only know arrays, objects, and for/while loops, so a lot of what you said is over my head, but I'll get to it eventually I guess! Thanks for the tips. –  user42212 May 10 at 0:59

Here one way to sum a list of integers using ECMAScript 5's functional support

// Sum all values in a list by reducing the list to a single value
array.reduce(function(prev, curr) {
  return prev + curr;
});

Not faster than your approach due to the repeated function calls, but definitely shorter.

Your approach can be further simplified:

// Move everything into the parenthesis of for-loop!
var sum = function(array) {
  for (
    var i = 0, len = array.length, total = 0; 
    i < len; 
    total += array[i++]
  );
  return total;
};

Not recommended because it's not very readable. I just think it's interesting :)

I'll give you an alternative approach to the "Find length of longest string" problem:

// Sort it by length descending and then get the first
var longestStrLen = array.sort(function(a, b) {
    a.length - b.length;
})[0].length;

Hope these open your eyes to some of the possibilities!

Now, here's an actual review:

var charFreq = function(string){
  var list = {}; // Not a list (implies order). Should name it a dictionary or map
  var length = string.length; // no need to have this be separate line
  for (var i = 0; i < length; i++) {  
  if (string.charAt(i) in list) 
    // What's with the +1??
    list[string.charAt(i)] += +1;
  else 
    // No need to call string.charAt multiple times!
    list[string.charAt(i)] = 1;
  }
  return list;
}

Revised version:

var charFreq = function(string){
  var dict = {}; 
  for (var i = 0, len = string.length; i < len; i++) {
    var currChar = string.charAt(i);  
    if (currChar in dict) { 
      dict[currChar] += 1;
    } else {
      dict[currChar] = 1;
    }
  }
  return dict;
}

Hope that helps!

share|improve this answer
    
I find ++ both easier to read and more idiomatic -- in particular for(i=0; i<X; i++) is probably one of the most common code snippets on the face of the planet, with the only minor variation in X and whether you have to declare i or not. –  jmoreno May 10 at 5:36
    
yeah I agree. It's definitely more idiomatic, esp. in a for-loop. Outside of a for loop I prefer using +=1 b/c I find it more explicit hence readable. –  linstantnoodles May 10 at 18:38

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, '$1o$1');
}

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 10× slower.


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.

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;
}
share|improve this answer

Try using '' instead of "" wherever possible. In most languages the content of the double quotes gets interpreted, so the single quote version is faster and does not leak variables.

I'm not sure if it is better, but you can define functions like this too:

function findLongestWord(array) {

instead of

var findLongestWord = function(array) {
share|improve this answer
2  
Perhaps you could add a little more explanation why changing the quotes would improve the code. It sounds to me just like a style that may vary from person to person, but where one is not objectively better than the other. –  icktoofay May 10 at 1:24
2  
Javascript does not have string interpolation so that does not affect anything here. –  cbojar May 10 at 5:20
    
Functional declarations in javascript are usually frowned upon because they are set at compile time rather than run-time which leads to unexpected behaviors. Usually better to use function expressions (var funcName = ...). –  linstantnoodles May 10 at 14:14
    
@cbojar there is string interpolation in coffeescript though <3 –  linstantnoodles May 10 at 14:17
    
Using single-quotes avoids the need to escape double-quotes when building HTML manually: '<a href="foo">' vs. "<a href=\"foo\">". –  David Harkness May 13 at 0:21

Your Answer

 
discard

By posting your answer, you agree to the privacy policy and terms of service.

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