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.

I'm playing Code Hunt on level 2.06 (loops). This level is about counting the number of "a" in a String.

public class Program {
public static int Puzzle(String s) {
   int ret = 0;
   for(int x = 0; x < s.length(); x++){
       if(s.charAt(x).equals('a')) ret++;
    }
return ret;
}

It actually works, but the game scores your code from 1 to 3 depending on the elegance of the code. Any ideas on improving that elegance?

share|improve this question
1  
Un-upvoting, and then Down-voting, because, as has been pointed out, the code does not compile. s.charAt(x).equals('a') is illegal. –  rolfl Jun 22 at 12:13
add comment

6 Answers 6

up vote 1 down vote accepted

You are using a loop when you could use an other method more concise and a bit "cleaner".

Try using the replace method defined on String objects and figure out how it could be useful to get what you want.

PS: The answer is a one-liner: aim for it!

share|improve this answer
1  
Given that this site is named CodeReview, is there any chance you could add more to your answer in the way of really reviewing the code in the question? The suggestion you've made might be part of it, but if you can expand it with any other comments you might have about the code, that could improve the answer tremendously. –  Jerry Coffin Jun 22 at 5:20
    
Wow, cool trick! –  11684 Jun 22 at 9:05
2  
When it makes sense to use 1-liners, I like them. In this case, your suggestion is 10 times slower than it needs to be. Don't suggest 1-liners when more efficient (hence more elegant) options exist. –  rolfl Jun 22 at 12:15
    
Indeed. Counting the number of occurrences should not create any new objects, since there's no need for it, unless you really insist on the hard-to-unterstand one-liner return s.length() - s.replace("a", "").length();. –  Roland Illig Jun 22 at 20:09
    
I have to downvote this: 1. Your Code should be clear and say what it does: Your code says replace, when what you want to do is count, so you need extra comments for someone else to understand. This is a 'smart' solution, the opposite of readable code and even slower than the straightforward approach! –  Falco Jun 23 at 10:50
add comment

This is a nice short piece of code, though it has a number of 'features' which can be improved.

Code Style

Java code style recommends a lower-case letter to start method names.... Puzzle should be puzzle, or better, getLetterCount. You may be limited by the testing tool though.

Efficiency

Inside the method, the biggest issue I have is the poor efficiency. A string in Java is essentially an array of 'char' primitive values: char[]

Your code takes each char, converts it to a Character (autoboxing), runs the equals('a') method on that Character, which converts the char 'a' to a Character a and does the equals on that.

It's normally more efficient to compare the primitives using == instead.

Consider the following code:

public class Program {
    public static int countAChars(String s) {
        int count = 0;
        for(int i = s.length() - 1; i >= 0; i--) {
            if('a' == s.charAt (i)) {
               count++;
            }
        }
        return count;
    }
}

Note: in the loop above I go backwards from the end of the String, which has occasionally been faster than going forwards, at least in my testing.

The following alternative code is more readable, but converts the String to an array first, which will be slightly slower....

public class Program {
    public static int countAChars(String s) {
        int count = 0;
        for(char c : s.toCharArray()) {
            if('a' == c) {
               count++;
            }
        }
        return count;
    }
}

Performance

I took a moment to code up the performance of a number of different solutions that have been posted so far. I used 'MacBeth' as an input file (this link will download the XML).

import java.io.IOException;
import java.nio.file.Files;
import java.nio.file.Paths;

public class Counter {

    private interface ACount {
        int countA(String line);

        String name();
    }

    private static final ACount[] COUNTERS = { new ACount() {

        @Override
        public int countA(String line) {
            int cnt = 0;
            for (int i = line.length() - 1; i >= 0; i--) {
                if (line.charAt(i) == 'a') {
                    cnt++;
                }
            }
            return cnt;
        }

        @Override
        public String name() {
            return "charAt()";
        }
    }, new ACount() {

        @Override
        public int countA(String s) {
            int ret = 0;
            for (int x = 0; x < s.length(); x++) {
                if (((Character) s.charAt(x)).equals('a'))
                    ret++;
            }
            return ret;
        }

        @Override
        public String name() {
            return "OPCode";
        }
    }, new ACount() {

        @Override
        public int countA(String line) {
            return line.length() - line.replace("a", "").length();
        }

        @Override
        public String name() {
            return "replace";
        }
    }, new ACount() {

        @Override
        public int countA(String line) {
            int cnt = 0;
            for (char c : line.toCharArray()) {
                if (c == 'a') {
                    cnt++;
                }
            }
            return cnt;
        }

        @Override
        public String name() {
            return "toCharArray";
        }
    }, };

    private static final int countAllA(ACount counter, String[] lines) {
        int cnt = 0;
        for (String line : lines) {
            cnt += counter.countA(line);
        }
        return cnt;
    }

    private static final void reportCounts(String name, String[] lines) {
        for (ACount counter : COUNTERS) {
            long start = System.nanoTime();
            int ac = countAllA(counter, lines);
            long nanos = System.nanoTime() - start;
            System.out.printf("%10s %12s %8d  %12.3fms%n", name,
                    counter.name(), ac, nanos / 1000000.0);
        }
        System.out.println();
    }

    public static void main(String[] args) throws IOException {
        String[] lines = Files.readAllLines(
                Paths.get(args.length == 0 ? "macbeth.xml" : args[0])).toArray(
                new String[0]);
        for (int i = 0; i < 100; i++) {
            reportCounts("Warmup " + i, lines);
        }
        reportCounts("Real Run", lines);

    }

}

This produces the results:

  Real Run     charAt()     5065         0.311ms
  Real Run       OPCode     5065         0.872ms
  Real Run      replace     5065         3.933ms
  Real Run  toCharArray     5065         0.387ms

The 1-liner answer you have accepted (and posted as your answer), is 10 times slower than the alternatives.

Your original code (if I cast the char to Character), is twice as slow as the alternatives I suggest.

What you determine to be elegant, is not, in my opinion, a good solution. If "Code Hunt" thinks the slow solution is the best solution, then I would find a different system for evaluating my code.

share|improve this answer
    
The code style is defined by the game, I can't change "Puzzle" to "puzzle". About the effiency: I considered all your suggestions but it was needed a bigger change to improve the efficiency. I've posted my new code. –  Frutal1ty Jun 22 at 10:25
    
I don't see any conversion from char to Character taking place, what I see is a compiler error. (See my answer) –  Simon André Forsberg Jun 22 at 11:32
    
I think the speed-difference in the loop could be resulting from the loop condition, because your String parameter is not final it has to re-evaluate (i < s.length()) in each iteration. I would prefer the forward loop, because a backwards loop only causes irritation and I just tested it - with final String line it is the same performance! –  Falco Jun 23 at 10:53
    
Hey @Falco - there is no i < s.length() in 'my' code, The backwards loop I recommend only calls int i = line.length() - 1 once (loop initializer). There is an x < s.length() which comes from the OP/Asker's code. Is that what you meant? –  rolfl Jun 23 at 11:19
    
Your code uses a backwards loop with i--! This is strange and irritating, so if you don't have a good reason, you should use a normal forward loop! ---> If your reason is performance, the performance is the same for a normal loop (i=0; i<line.length(); ++i) if line is final. –  Falco Jun 23 at 11:21
show 2 more comments

Does that code really compile?

Rolfl says that you're converting the char to Character, I however don't see any indication of that.

s.charAt(x) gives you a char, you can't use .equals on a char.

This line:

if(s.charAt(x).equals('a')) ret++;

Causes compiler error:

Cannot invoke equals(char) on the primitive type char

If you don't get that compiler error in the Code Hunt application, then Code Hunt apparently does things differently.


Your indentation is severely off, this might not be something that Code Hunt give score for, but it is what humans pay attention to.

Also, don't write the statement for when if is true on the same line, and preferably use braces (I know, I know, I don't like that advice myself when it's only one line but I'm starting to get used to the convention by now).

Here is how I would have written your original code, stylistically:

public class Program {
    public static int Puzzle(String s) {
        int ret = 0;
        for (int x = 0; x < s.length(); x++) {
            if (s.charAt(x).equals('a')) {
                ret++;
            }
        }
        return ret;
    }
}

As for the approach itself, I think using a for loop to count the variables "old-style" is better than using String.replace as it doesn't involve creation of any new objects.

I believe Code Hunt doesn't score the actual run-time performance here, I think it scores based on which kind of code you use for it, whether or not you use a for-loop and such. You said that the level is called level 2.06 (loops) so one would expect loops to be used. When it comes to performance, I believe rolfl's code is more efficient than the String.replace.

share|improve this answer
    
Its not necessary to use loops. –  Frutal1ty Jun 22 at 12:06
3  
@Frutal1ty - I updated my answer with performance results. Also, it is necessary to use loops. The 1-liner replace method uses multiple loops (how do you think replace is implemented?). In this case, replace uses Regex and complex string manipulation to get the job done. –  rolfl Jun 22 at 12:29
add comment

I don't know which is the meter for elegance but whatever.

  1. Choose better names for you variables, what ret means? It's the sum of all "a" inside a String! Why not call it sumOfA or aCount? (They are terrible as nanes, but I can't get something better right now...)

  2. It's one level for, why don't call the variable i instead of x? I know it's short too, but i in for remember something like "index" which is what you do inside (you get the char at index x).

  3. I don't know if it can be improved to remove method call overhead, but you could cache .length () of the string to avoid to execute the call every time.

Nothing more to say (for me).

It's how it could be, I'm at phone and I can't see if the code looks good so I will fix it (and sorry for the variable name countOfA, it's horrible and I doubt a name like this would be used really but... as I said... no more ideaa on how to call it.)

public class Program {
    public static int Puzzle(String s) {
        int countOfA = 0;
        for(int i = 0; i < s.length(); i++) {
            if('a' == s.charAt (i)) {
               countOfA++;
            }
        }
    return countOfA;
    }
}

Point 3 was wrong.

share|improve this answer
add comment

I'm not sure if you are allowed to use Java 8, but in case you can this is my one liner version using the new stream API and lambdas:

public long countACharsJ8() {
    return s.chars().filter(c -> c == 'a').count();
}

I have not done any performance tests but the times should be similar to the charAt() or toCharArray speed measured by rolfl.

share|improve this answer
add comment

Thanks to Cranium to make me look the problem from another point of view, without any loop. There's my new code:


     public class Program {
        public static int Puzzle(String s) {
            String s1 = s.replace("a","");
            return s.length()-s1.length(); 
        }
    }

 
share|improve this answer
    
Or: s.replaceAll("[^a]", "").length() (Although I like @rolfl's solution the most, as that's not using any String replacement) –  Simon André Forsberg Jun 22 at 11:19
2  
In my analysis of how replace works, you actually have about 4 or so loops. –  rolfl Jun 22 at 12:36
add comment

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.