11
\$\begingroup\$

I am taking an introductory course in java in high school, however I have some experience in python. I was tasked to find the number of times a character occurs in a string so I did this.

String a = "hello";
String letter = "l";
int occurrence = a.length() - a.replace(letter, "").length();

Happy that I came up with a clever one-liner, I showed it to my teacher and she said that it's more elegant to use a for loop. What does she mean by that? How do we define elegance in code? Is it subjective? As an aside is this more efficient than a for loop or vice-versa?

\$\endgroup\$
  • 2
    \$\begingroup\$ Welcome to Code Review! I changed the title so that it describes what the code does per site goals: "State what your code does in your title, not your main concerns about it.". Feel free to edit and give it a different title if there is something more appropriate. \$\endgroup\$ – Sᴀᴍ Onᴇᴌᴀ May 2 '18 at 19:36
  • 1
    \$\begingroup\$ @Pheo, I disagree with your initial assessment that Code Maker is "not asking for any code to be reviewed". I think that this is a small piece of code and that they are curious as to whether or not their code is as efficient, or if it is better than, a for loop. In my opinion I consider this a review of their one-liner. \$\endgroup\$ – Malachi May 2 '18 at 19:57
  • \$\begingroup\$ I don't know if I would call it "elegance", or lack thereof, but what I find unsatisfactory about your code is that it does more than it needs to, because it creates a new String object. A for loop might yield more code, but functionality-wise, it would be more to the point. Maybe this is what your teacher meant. \$\endgroup\$ – Stingy May 2 '18 at 20:21
  • \$\begingroup\$ If you want a real “one liner” I have to point out the example in Perl’s intro manual: $cnt = $sky =~ tr/*/*/; # count the stars in $sky \$\endgroup\$ – JDługosz May 3 '18 at 8:57
10
\$\begingroup\$

To explain what's going on here, perhaps a real world analogy would help.

You have a book, and have to count the number of times "e" appears in the book. Your solution is to copy the contents of the book into another book, letter by letter. Then you go through your copy of the book, and leave out each "e". Next you count the number of letters in each book, and the difference is the number of times "e" appeared. That works, but it uses up an awful lot of paper and an awfully long time!

To some extent you're right that "elegance" is a subjective concept. As a rule of thumb, elegant code generally starts from thinking about the problem and understanding the fundamental thing that you are trying to do. It then does that, as straightforwardly (in terms of being understandable to humans) as it can and as efficiently (in terms of minimizing the time, memory, and other resources) as it can. (The subjectivity comes when deciding which of those considerations is more important)

One thing that is nice about your solution is that you're willing to use features from the Java standard library. A lot of student programmers are determined to write everything from scratch in terms of for loops and if statements. Using the inbuilt library is an excellent habit to get into. It just so happens that in this case the library functionality you're using is quite expensive for the problem that you're trying to solve, and is not as straightforward that it's counting characters as a loop, an if, and a tally would have been.

\$\endgroup\$
10
\$\begingroup\$

I am not sure I agree that a for loop is more elegant per se. The downside of your method compared to a traditional for loop is that it requires a bit more processing most of the time, because you need to create a new character buffer and put the replacements in there, and you create a new String object.

I would probably create an initial implementation with a Java 8 Stream, e.g.:

String string = "hello";
char letter = 'l';
long count = string.chars().filter(character -> character == letter).count();

To me this is probably more elegant than a traditional for loop. With Streams it is possible to write it in a declarative way. In a lot of cases they are also trivial to parallelize, requiring only a parallel function call on the Stream. Furthermore, they are lazy which means the processing can stop early, only the necessary data will be processed. Note that in this case, we have to process each element (each character), so it makes no difference here.

\$\endgroup\$
  • 1
    \$\begingroup\$ what are the benefits of a Stream? I am curious. \$\endgroup\$ – Malachi May 2 '18 at 20:12
  • \$\begingroup\$ Part subjective, but I like the declarative aspect of it. For me it is very easy to read what it is doing, i.e. count the occurences of a single character in a String. In a lot of cases it is also trivial to parallelize by just calling parallel on the Stream, though I am not sure if it would help much in this case. \$\endgroup\$ – Koekje May 2 '18 at 20:21
  • \$\begingroup\$ Interesting, would you include that with your answer please, for posterity? \$\endgroup\$ – Malachi May 2 '18 at 20:32
  • 1
    \$\begingroup\$ @Malachi I seem to recall that streams are also lazy, so it's possible to only partially evaluate the stream. (In this case, we need the whole thing, but we could potentially build a stream that could short-circuit if we only need to know that there are, for example, at least 7 instances.) \$\endgroup\$ – 410_Gone May 2 '18 at 20:36
  • 2
    \$\begingroup\$ Correct, this also allows us to operate on infinite sequences. @Malachi, I will update my answer in a bit. \$\endgroup\$ – Koekje May 2 '18 at 20:47
10
\$\begingroup\$

You're lucky that the task is to count the occurrences of a single character. Your code instead looks as if it would count the occurrences of a string.

The code, as it is currently, only works for strings of length 1. Curiously, this excludes emojis:

String text = "hello 😀😀😀😀😀😀😀";
String chr = "😀";
int occurrence = text.length() - text.replace(chr, "").length();

This code counts 14, but there are only 7 emojis. Therefore you have to divide by the length of the searched string:

int occurrence = (text.length() - text.replace(chr, "").length()) / chr.length();

An emoji looks like a single character, but it occupies two char places in a string, for historical reasons. Java stores strings in the UTF-16 encoding. There's much more to learn about this topic, and it's a good idea to become knowledgeable in this area.

Except from this issue, your code is clever and gets its job done. The for loop is more complicated to read but probably a bit faster. Anyway, you should always write automated tests for your code, and these tests will be a lot simpler than your clever one-liner. Therefore, having this cleverness is ok.

public class OccurrenceTest {

  @Test
  public void testEmoji() {
      String text = "hello 😀😀😀😀😀😀😀";
      String chr = "😀";
      int occurrence = text.length() - text.replace(chr, "").length();

      assertEquals(7, occurrence); // FIXME: currently it is 14 since the emoji is 2 chars long
  }
}
\$\endgroup\$
  • 1
    \$\begingroup\$ +1 for the advice to test code, as well as the interesting edge case. It might be more appropriate for the example test to call a function that contains the character counting, and for the assertEquals to require the expected behaviour (and so be a failing test). \$\endgroup\$ – Josiah May 2 '18 at 20:30
  • \$\begingroup\$ It's an introductory course and they will learn writing functions soon enough. I therefore concentrated on the main ideas and concepts. The chances are higher that these more advanced concepts are not covered. \$\endgroup\$ – Roland Illig May 3 '18 at 5:06
  • 2
    \$\begingroup\$ I would modify the test example to assertEquals(7, occurence); //Test fails since occurence = 14 A test should not be green when the code under test is wrong. \$\endgroup\$ – Imus May 3 '18 at 9:13
5
\$\begingroup\$

Understanding parameters

You're lucky that the task is to count the occurrences of a single character. Your code instead looks as if it would count the occurrences of a string.

(@RolandIllig's answer)

I think that's a good start to understanding why defining a method with the right parameters is important. The original task is to count character occurrences, so it's likely the method should look something like this in Java:

public static int countOccurrence(String input, char value) { /* ... */ }
// to call
countOccurrence("hello", 'l');

In that case, your implementation will then change slightly to become:

public static int countOccurrence(String input, char value) {
    return input.length() - input.replace(String.valueOf(value), "").length();
}

By making it clear what your parameters are, we can now care about how to explain your logic.

Explaining logic vs implementation

Similar to what was pointed out in @Josiah's answer, have a think about how you will explain your logic, and then reflect if that was as elegant as saying "loop through each character and count the number of matches for my 'value' character".

It's easy to commingle short code as elegant code, but as you have to deal with more complex logic in larger code bases, you will learn that you have to care about how it can be explained rather than how short it reads.

It is not as elegant to understand a simple 'count-the-occurrence' method as having to get the length of the input string, and subtract that with a copy that does not have the matching 'value' character.

Performance

To be frank, you really should not have to care about performance for a simple method as this, as that reeks of micro-optimization. However, if you are interested in comparing your solution and a simple for loop, you can roughly think in terms of the memory required by your code as it goes along.

Your solution already knows about its input, and it needs to create a copy of that input, albeit shorter. A for loop requires an additional int to count. It doesn't require making that extra copy.

On the other hand, the stream-able way has more 'plumbing' underneath, but its advantage come in terms of a more declarative way of approach, instead of the imperative approaches we traditionally have.

\$\endgroup\$
1
\$\begingroup\$

When reading through code one shouldn't be surprised about how something is implemented. The best pieces of code are those that make you think "well ofcourse it's written like this".

When we look at the assignement:

count how many letters of the input string are equal to the wanted character

which of these solutions sounds least surprising:

loop through each character in the input string and increment the counter if it's equal to the wanted character.

or

remove all characters equal to the wanted character from the original string. Substract the length of the remaining string from the original strings lenght.

or to include the stream option of Koekje

go through all the characters of the input string and keep only those characters equal to the wanted character. Count how many are left.

I personally prefer the really simple for loop since it reads almost like the assignment and I can understand how other people might prefer the stream solution. Your solution on the other hand takes an extra second or 2 to understand what it does exactly.

This is most likely also what your teacher means when she said the for loop solution is more "elegant".


Being clever when writing a piece of code doesn't help someone else (or you in a couple of years) to understand that piece of code when reading it. If you want some extreme examples of this, look at some codegolf solutions. I doubt any of the authers of those answers would be able to tell what it does when they read those extremely clever oneliners again after a couple of years.

\$\endgroup\$

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.