8
\$\begingroup\$

Challenge:

Write a program that will clean up the words from extra numbers and symbols.

Specifications:

The first argument is a path to a file.
Each line includes a test case with a list of words: letters are both lowercase and uppercase, and are mixed with extra symbols.
Print the words separated by spaces in lowercase letters.
The length of a test case together with extra symbols can be in a range from 10 to 100 symbols. The number of test cases is 40.

Solution:

import java.io.File;
import java.io.FileNotFoundException;
import java.util.Scanner;

public class CleanWords {
    public static void main(String[] args) {
        try (Scanner input = new Scanner(new File(args[0]))) {
            while(input.hasNextLine()) {
                System.out.println(sanitize(input.nextLine()));
            }
        } catch (FileNotFoundException fnfe) {
            System.out.println(args[0] + " doesn't exist");
        }
    }

    private static String sanitize(String input) {
        StringBuilder result = new StringBuilder();
        for (String s : input.split("[^a-zA-Z]+")) {
            result.append(' ').append(s);
        }
        return result.toString().toLowerCase().trim();
    }
}

Sample Input:

(--9Hello----World...--)  
Can 0$9 ---you~  
13What213are;11you-123+138doing7

Sample Output:

hello world  
can you  
what are you doing

I'm wondering if:

  1. It would be more efficient to call trim within the loop?
  2. If there's a less costly way than looping twice between calling both toLowerCase and trim

These may seem like givens, but it's been a long time since I did one of these, and I wanted to get back into it with a seemingly simple one. I passed on the first try which I largely have CR to thank for. For those interested, the source is CodeEval.com.

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

This can be significantly cleaned up and simplified using Java 8. Let's start with a quick review, though before rewriting it all :)

public static void main(String[] args) {
    try (Scanner input = new Scanner(new File(args[0]))) {

You perform no validation whatsoever here. When this is called without an argument, you'll get a ArrayIndexOutOfBoundsException... not nice. Validate your inputs (even when the assignment says you'll get a path)

Also you should consider using JSR-203 (aka. java.nio) API. It's cleaner and more informative wrt. Exceptions and general handling of problematic inputs. Also it integrates very nicely with Streams :D

But you're using try-with-resources, which is a good start.

        while(input.hasNextLine()) {
            System.out.println(sanitize(input.nextLine()));
        }

This is a small indication your main-method is doing too much. You do:

  • Open file
  • read that file
  • call sanitize
  • print
  • error handling

Following the Single Responsibility Principle by the letter would mean every single one of these should be a separate method. I personally find that overkill, but I'd at least shift the responsibilities a little by separating result retrieval (aka. sanitize()) from printing.

Which gets me to sanitization or rather how you do it:

    for (String s : input.split("[^a-zA-Z]+")) {

What you're doing here is interesting and basically a cool trick, but.. you're cheating around what you actually want.

You want to extract words consisting of [a-z] (case insensitive). What you do is split you String on everything else and then reassemble it.

So there's a few things you could've done instead. First possibility would've been to extract the words with a Pattern and then assembling them, which might've been more obvious:

Pattern words = Pattern.compile("([a-z]+)", Pattern.CASE_INSENSITIVE);
Matcher m = words.matcher(s);
StringBuilder sb = new StringBuilder();
while (m.find()) {
   sb.append(m.group()).append(' ');
}

This basically accomplishes the same as your code, but is more explicit as to what you want to match. But this is a lot of code for something dead simple. It boils down to what your split does in combination with the loop.

What you do is replace everything, that's not a latin letter with a space. Why not write that?

s.replaceAll("[^a-zA-Z]+", " ");

this eliminates the for-loop you have and .. basically does the same :D

Now that we got this out of the way, let's look at how it looks with JSR203 and Streams :)

public static void main(String[] args) {
    try (Stream<String> lines = Files.lines(Paths.get(args[0]))) {
        final String output = lines.map(CleanWords::sanitize)
             .collect(Collectors.joining(System.lineSeparator()));
        System.out.println(output);
    } catch (IOException ex) {
        System.out.println("Could not read File");
    }
}

private static String sanitize(String input) {
     return input.replaceAll("[^A-Za-z]+", " ").toLowerCase().trim();
}
\$\endgroup\$
  • 3
    \$\begingroup\$ +1, though I disagree about the input validation. It's simply unnecessary here, so don't sweat it. I wouldn't even handle the exception at all, and declare main to throw instead. \$\endgroup\$ – janos Jul 19 '15 at 16:34
  • \$\begingroup\$ I was tentative about posting this since I couldn't see much room for improvement. But your answer exemplifies why I love this site. There's always something that can be bettered and truly skilled senpais to show you the way, thank you for the spectacular answer. \$\endgroup\$ – Legato Jul 19 '15 at 18:50
  • \$\begingroup\$ I just realized this, but if you're joining() by the line separator and then sending the result to System.out.println(), you probably should use forEach(System.out::println). \$\endgroup\$ – h.j.k. Jul 20 '15 at 2:55
  • \$\begingroup\$ @h.j.k then again that would be intermingling responsibilities \$\endgroup\$ – Vogel612 Jul 20 '15 at 4:36
  • \$\begingroup\$ @Vogel612 how so? In this specific example, it can be part of the stream processing. Unless you mean if we have abstracted the processing into its own method (and returning a String for example)... \$\endgroup\$ – h.j.k. Jul 20 '15 at 5:40
1
\$\begingroup\$

Regarding naming:

My first thought about a class named CleanWords without looking at its JavaDoc or implementation would be: "This class could implement Collection or extend one of the collection classes to contain clean words.".

My second thought after seeing the beginning of the class declaration without any of these: "This looks more like a name of a method that cleans words.".

I'd name the class WordCleaner and the method cleanUp(). Such also better reflecting the challenge description.

\$\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.