patternjavaMinor
Matching words with a scrambled word
Viewed 0 times
withwordsscrambledwordmatching
Problem
I have this code that reads in a "scrambles" and a "words" file. It compares both files using their canonical form and prints out the words from the "words" file that match a particular scrambled word. I'm looking for ways to make this quicker.
```
public class scramble
{
public static void main(String[] args) throws Exception
{
if (args.length scramblesList= new ArrayList(); // default initial capacity is 10
ArrayList wordList= new ArrayList();
long startTime = System.currentTimeMillis(); // like clicking START on your stopwatch
while (scrambles.ready())
{
String scramble = scrambles.readLine();
scramblesList.add( scramble );
}
Collections.sort( scramblesList );
while (words.ready())
{
String word = words.readLine();
dictionaryList.add( word );
}
Collections.sort( wordList );
for(String scramble: scramblesList)
{
System.out.print(scramble + " ");
for(String word: wordList)
{
if(toCanonical(scramble).equals(toCanonical(word)))
System.out.print(word + " ");
}
System.out.println();
}
long endTime = System.currentTimeMillis(); // like clicking STOP on your stopwatch
long ms = endTime-startTime;
System.out.println("Elapsed time in seconds: " + ms/1000.0 + "\n"); // 1 ms is a 1,000th of a second
} // END MAIN
private static void die( String errmsg )
{
System.out.println( "\nFATAL ERROR: " + errmsg + "\n" );
System.exit(0);
}
private static String toCanonical( String word )
{
char[] array = word.toCharArray();
// #1 declare a char[] array and assign into it the return value from word.toCharArray()
Arrays.sort(array);
// #2 pass that char[] into Arrays.sort(...)
String C
```
public class scramble
{
public static void main(String[] args) throws Exception
{
if (args.length scramblesList= new ArrayList(); // default initial capacity is 10
ArrayList wordList= new ArrayList();
long startTime = System.currentTimeMillis(); // like clicking START on your stopwatch
while (scrambles.ready())
{
String scramble = scrambles.readLine();
scramblesList.add( scramble );
}
Collections.sort( scramblesList );
while (words.ready())
{
String word = words.readLine();
dictionaryList.add( word );
}
Collections.sort( wordList );
for(String scramble: scramblesList)
{
System.out.print(scramble + " ");
for(String word: wordList)
{
if(toCanonical(scramble).equals(toCanonical(word)))
System.out.print(word + " ");
}
System.out.println();
}
long endTime = System.currentTimeMillis(); // like clicking STOP on your stopwatch
long ms = endTime-startTime;
System.out.println("Elapsed time in seconds: " + ms/1000.0 + "\n"); // 1 ms is a 1,000th of a second
} // END MAIN
private static void die( String errmsg )
{
System.out.println( "\nFATAL ERROR: " + errmsg + "\n" );
System.exit(0);
}
private static String toCanonical( String word )
{
char[] array = word.toCharArray();
// #1 declare a char[] array and assign into it the return value from word.toCharArray()
Arrays.sort(array);
// #2 pass that char[] into Arrays.sort(...)
String C
Solution
There are a bunch of things in here that @nanny has not yet covered, so let's go through some of those:
-
Java class names should be CamelCase. Your class
-
use whitespace in lines consistently, especially around keywords and operators. Lines like:
should be:
You have done this correctly in most places, so the places you have it wrong stand out....
-
There is no need, and it is now discouraged, for you to add the generic type to instance constructor calls. The generic type can be inferred from the target. i.e. your line:
should be:
-
When possible, you should use the interface type for your variables, not the concrete type. For example, the line above should actually be:
-
No need to "type" variable names ... (hungarian notation). We know that
-
1-liners are a maintenance problem. Sure, they look smart, and take less space, but the person maintaining your code in the furture will dislike you. This:
should be:
-
while we are on it, you have successfully turned your Java semantics in to Perl... was that the intention? If so, there's better ways to handle problems like this... an
-
try-with-resources is your friend, learn to use it: try-with-resources
-
Know your standard libraries... Java can do a lot of what you are doing in it's native libraries. Learn them....
-
use
can be:
Oh, and you have added a
Let Java work for you:
This code throws lots of potential exceptions which you don't handle... and there's a lot simpler ways to do things....:
The above code, excluding the sorts, and stopwatch, can be replaced with:
As @nanny pointed out, the sorting is unnecessary.
Performance
Once you have your two word lists, the fastest way to match these things is to use a map...., a map of Lists....
Convert the words in to a
Then, with canonWords, search for matches with scrambled words:
-
Java class names should be CamelCase. Your class
scramble should be Scramble.-
use whitespace in lines consistently, especially around keywords and operators. Lines like:
... wordList= new ....should be:
... wordList = new ....You have done this correctly in most places, so the places you have it wrong stand out....
-
There is no need, and it is now discouraged, for you to add the generic type to instance constructor calls. The generic type can be inferred from the target. i.e. your line:
ArrayList scramblesList= new ArrayList();should be:
ArrayList scramblesList = new ArrayList<>();-
When possible, you should use the interface type for your variables, not the concrete type. For example, the line above should actually be:
List scramblesList = new ArrayList<>();-
No need to "type" variable names ... (hungarian notation). We know that
scramblesList is a list, so there's no need for the List suffix on scramblesList, it should just be scrambles (good thing we renamed the class to Scramble to avoid confusion...).-
1-liners are a maintenance problem. Sure, they look smart, and take less space, but the person maintaining your code in the furture will dislike you. This:
if (args.length < 2 ) die( "Must pass name of input files on cmd line." );should be:
if (args.length < 2 ) {
die( "Must pass name of input files on cmd line." );
}-
while we are on it, you have successfully turned your Java semantics in to Perl... was that the intention? If so, there's better ways to handle problems like this... an
IllegalArgumentException is the better/right solution.-
try-with-resources is your friend, learn to use it: try-with-resources
-
Know your standard libraries... Java can do a lot of what you are doing in it's native libraries. Learn them....
-
use
printf:long endTime = System.currentTimeMillis(); // like clicking STOP on your stopwatch
long ms = endTime-startTime;
System.out.println("Elapsed time in seconds: " + ms/1000.0 + "\n");can be:
long ms = System.currentTimeMillis() - startTime;
System.out.printf("Elapsed time in seconds: %.3f\n", ms / 1000.0);Oh, and you have added a
\n to a println which seems redundant.Let Java work for you:
This code throws lots of potential exceptions which you don't handle... and there's a lot simpler ways to do things....:
BufferedReader scrambles = new BufferedReader( new FileReader( args[1] ) );
BufferedReader words = new BufferedReader ( new FileReader( args[0] ) );
ArrayList scramblesList= new ArrayList(); // default initial capacity is 10
ArrayList wordList= new ArrayList();
long startTime = System.currentTimeMillis(); // like clicking START on your stopwatch
while (scrambles.ready())
{
String scramble = scrambles.readLine();
scramblesList.add( scramble );
}
Collections.sort( scramblesList );
while (words.ready())
{
String word = words.readLine();
dictionaryList.add( word );
}
Collections.sort( wordList );The above code, excluding the sorts, and stopwatch, can be replaced with:
List words = Files.readAllLines(Paths.get(args[0]));
List scrambles = Files.readAllLines(Paths.get(args[1]));As @nanny pointed out, the sorting is unnecessary.
Performance
Once you have your two word lists, the fastest way to match these things is to use a map...., a map of Lists....
Convert the words in to a
Map>:Map> canonWords = new HashMap<>();
for (String word : words) {
String canon = toCanonical(word);
if (!canonWords.containsKey(canon)) {
canonWords.put(canon, new ArrayList<>());
}
canonWords.get(canon).add(word);
}Then, with canonWords, search for matches with scrambled words:
for (String scram : scrambles) {
String canon = toCanonical(scram);
List match = canonWords.get(canon);
if (match != null) {
System.out.printf("Scramble %s Matches words %s\n", scram, match.toString());
}
}Code Snippets
... wordList= new ....... wordList = new ....ArrayList<String> scramblesList= new ArrayList<String>();ArrayList<String> scramblesList = new ArrayList<>();List<String> scramblesList = new ArrayList<>();Context
StackExchange Code Review Q#84589, answer score: 8
Revisions (0)
No revisions yet.