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.
s.charAt(x).equals('a')
is illegal. – rolfl♦ yesterday