Imprimiendo las palabras más usadas de frases -- java campo con performance camp codereview Relacionados El problema

Printing the most used words from phrases


11
vote

problema

Español

Tengo una colección de frases en un List . Cada frase es un String Array donde cada elemento en la matriz es una palabra.

He creado un List<Entry<String, Integer>> que posee el words como las teclas y el 9988777665544335 como valor. Todo está ordenado por valor en orden descendente.

Entonces, lo que hago es imprimir el X palabras que se utilizaron junto con la cantidad de veces que se utilizaron. Lo que quiero saber es si hay una forma mejor / más simple de hacerlo y, en general, cualquier cosa que desee agregar para que mi código se vea o mejore mejor.

aquí es mi código:

  public class WordCounting {      public static void printTopWords(final int numberOfWords, List<String[]> phrases) {         List<Entry<String, Integer>> wordsMap = entriesSortedByValues(wordCount(phrases));          Iterator entries = wordsMap.iterator();         int wordsCounter = 1;         while (entries.hasNext() && wordsCounter <= numberOfWords) {             Entry entry = (Entry) entries.next();             String key = (String) entry.getKey();             int value = (int) entry.getValue();              System.out.println(wordsCounter + ": " + key + " - " + value);             wordsCounter++;         }     }      private static Map<String, Integer> wordCount(List<String[]> phrases) {         Map<String, Integer> wordCounter = new TreeMap<>();          for (String[] strings : phrases) {             for (String string : strings) {                 wordCounter.put(string, wordCounter.get(string) == null                         ? 1 : wordCounter.get(string) + 1);             }         }         return wordCounter;     }      static <K, V extends Comparable<? super V>>             List<Entry<K, V>> entriesSortedByValues(Map<K, V> map) {          List<Entry<K, V>> sortedEntries = new ArrayList<>(map.entrySet());          Collections.sort(sortedEntries,                 new Comparator<Entry<K, V>>() {                     @Override                     public int compare(Entry<K, V> e1, Entry<K, V> e2) {                         return e2.getValue().compareTo(e1.getValue());                     }                 }         );          return sortedEntries;     } }   
Original en ingles

I have a collection of phrases in a List. Each phrase is a String Array where each element in the array is a word.

I create a List<Entry<String, Integer>> which holds the words as keys and the times used as value. Everything is sorted by value in descending order.

Then what I do is print the top X words that were used along with how many times they were used. What I want to know is if there is a better/simpler way of doing it and generally anything you want to add to make my code look or perform better.

Here is my code:

public class WordCounting {      public static void printTopWords(final int numberOfWords, List<String[]> phrases) {         List<Entry<String, Integer>> wordsMap = entriesSortedByValues(wordCount(phrases));          Iterator entries = wordsMap.iterator();         int wordsCounter = 1;         while (entries.hasNext() && wordsCounter <= numberOfWords) {             Entry entry = (Entry) entries.next();             String key = (String) entry.getKey();             int value = (int) entry.getValue();              System.out.println(wordsCounter + ": " + key + " - " + value);             wordsCounter++;         }     }      private static Map<String, Integer> wordCount(List<String[]> phrases) {         Map<String, Integer> wordCounter = new TreeMap<>();          for (String[] strings : phrases) {             for (String string : strings) {                 wordCounter.put(string, wordCounter.get(string) == null                         ? 1 : wordCounter.get(string) + 1);             }         }         return wordCounter;     }      static <K, V extends Comparable<? super V>>             List<Entry<K, V>> entriesSortedByValues(Map<K, V> map) {          List<Entry<K, V>> sortedEntries = new ArrayList<>(map.entrySet());          Collections.sort(sortedEntries,                 new Comparator<Entry<K, V>>() {                     @Override                     public int compare(Entry<K, V> e1, Entry<K, V> e2) {                         return e2.getValue().compareTo(e1.getValue());                     }                 }         );          return sortedEntries;     } } 
     
         
         

Lista de respuestas

12
 
vote
vote
La mejor respuesta
 

Como otros han señalado las mejoras más obvias, me gustaría hablar sobre Java 8. Tal vez no pueda usarlo todavía, pero lo recomendaría mirarlo y esta respuesta debe ser útil para cualquier persona que lee esto.

Mi punto de enfoque principal aquí estará diseñando el código de tal manera que lógicamente hará lo que se supone que debe hacer.

Tiene un método en el que tome un 99887766555443356 como argumento, y desea devolver las presentes presentes X, eso es todo lo que desea, en orden que significa:

  1. Pon todo LinkedList temp = head; while (temp.next != null) { // ... temp = temp.next; 7 s juntos en alguna estructura.
  2. Mantenga un registro de cuántas ocurrencias tienen.
  3. Ordenar la lista para tener las palabras con la mayor cantidad de ocurrencias primero.
  4. Devuelve el Top X ocurrencias.
  5. luego decide lo que quieres hacer con eso.

El número 4 y 5 se separan explícitamente aquí, porque eso es lo que debes hacer. Cada método debe servir de un propósito y hacer algo y imprimiendo, no es un propósito.

Otra mejora es que, como la entrada, quiero una estructura que consiste en LinkedList temp = head; while (temp.next != null) { // ... temp = temp.next; 8 s, y no su (ulgy) 99887776655443359 , también trataré con eso. El código se explicará completamente a continuación. Mientras estamos en ello, también mejoraré la comprobación de errores.

El código:

  add0  

  add1  

La salida:

1: a - 3
2: B - 2

La explicación:

  1. TIENE SU add2655443362 Frases Primero, desea simplemente tener un objeto que posee su add3 s. Aquí un add4 es un objeto adecuado, ya que solo necesita tener una vista en su objeto 99887776655443365 , no tiene sentido almacenar en realidad el almacenamiento de la nuevos datos. Usted hace esto llamando add6 .
    1. Esto primero girará su add7 en un add8 .
    2. Luego usas una referencia de método que describe el Lambda add9 para adquirir un addLast0 .
    3. Luego, con el addLast1 Agregue todos los elementos del addLast2 Volver al original addLast3 .
  2. Luego, comienza en el método addLast6655443374 , que devuelve un addLast5 , nuevamente para ofrecer la flexibilidad para hacer lo que desea con los resultados, no son no almacenado todavía en el punto donde se devuelve.
  3. Primero agregué alguna revisión de errores.
  4. Luego obtengo un 99887766655443376 que comparará cuál es la entrada con el número más bajo . Esto se hace utilizando un addLast7 en el valor de la entrada, que se obtiene con la referencia del método addLast8 .
  5. Luego inicio la cadena de operaciones en la entrada addLast9 :
    1. Primero grupo Los resultados por su identidad, que normalmente produce un addFirst0 .
    2. El truco aquí es que también usé un downstream addFirst1 , que cuenta el número de veces que ocurre la cadena, por lo tanto, se llama 99887776655443382 .
    3. En este punto, tengo un addFirst3 que denota la palabra y el número de ocurrencias. Utiliza un 99887766655443384 , porque esto es lo que devuelve.
    4. Luego obtengo un addFirst6 y convertirlo en un flujo.
    5. Luego, llamo addFirst7 en el addFirst8 con el comparador revertido . Esto se hace aquí, porque la interferencia de tipo no es lo suficientemente fuerte como para usar addFirst9 .
    6. Luego, addLast0 la corriente por los elementos principales X.
  6. Ahora tenemos el addLast1 , y aquí decidimos recolectarlo en un addLast2 .
  7. Aquí continuamos con la vieja lógica de tener un contador adjunto a él.

Unos pocos puntos que vale la pena notar:

  1. El 998877666554433933 es feo, pero es necesario no dar lugar a la fundición de tipo, que es aún más feo, es una limitación de la interferencia de tipo actual. Puede ser solo un problema en el compilador de IDE's y el compilador Javac podría compilarlo.
  2. El uso de addLast4 está bastante hinchado, pero nuestra opción más razonable, además de crear una clase 99887766655443395 998877665544 396 . Esperamos que esto sea más fácil si Java 9 incluye tuplas (que incluyen lógicamente pares) como ciudadanos más o menos de primera clase.
  3. En todo el método addLast7 Terminamos almacenando todas las entradas en la memoria una vez, con el addLast8 , estoy bastante seguro de que hay formas de evitar eso, pero no vale la pena El esfuerzo aquí, solo optimiza esto si se convierte en un verdadero cuello de botella.
  4. Estaba esperando usar el método addLast9655443399 al procesar los resultados, sin embargo, esto no es posible con el requisito de que desee tener un contador . Una vez más, más posibilidades se abren en Java 9 cuando esperamos 998877766554433100 S y Tuples.

Espero que esta revisión haya sido útil para usted.

 

As others have pointed out the most obvious improvements already, I would like to talk about Java 8. Maybe you cannot use it yet, but I would recommend looking into it and this answer should proivde useful for anyone reading this.

My main focus point here will be designing the code such that it will logically do what it is supposed to do.

You have a method in which you take a List<String[]> as argument, and you want to return the top x occurences, that is all you want, in order that means:

  1. Put all Strings together in some structure.
  2. Keep track of how many occurences they have.
  3. Sort the list to have the words with the highest amount of occurences first.
  4. Return the top x occurences.
  5. Then decide what you want to do with it.

Issue 4 and 5 are seperated explicitely here, because that is what you ought to do. Every method should serve one purpose, and doing something and printing it is not one purpose.

Another improvement is that as input I want a structure consisting of Strings, and not your (ugly) List<String[]>, I will also deal with that. The code will be fully explained below. While we are at it, I'll also improve the error checking.

The code:

public static Stream<Map.Entry<String, Long>> getTopWords(final int topX, final Stream<String> words) {     if (topX < 1) {         throw new IllegalArgumentException("invalid value for topX: " + topX);     }     Objects.requireNonNull(words);     Comparator<Map.Entry<String, Long>> comparator = Comparator.comparingLong(Map.Entry::getValue);     return words.collect(Collectors.groupingBy(i -> i, Collectors.counting()))             .entrySet().stream()             .sorted(comparator.reversed())             .limit(topX); } 

List<String[]> phrases = Arrays.asList(new String[]{"a", "b", "c"}, new String[]{"a", "a", "b", "d"}); List<Map.Entry<String, Long>> topEntries = getTopWords(2, phrases.stream().flatMap(Arrays::stream))         .collect(Collectors.toList()); int counter = 1; for (Map.Entry<String, Long> entry : topEntries) {     System.out.println(counter + ": " + entry.getKey() + " - " + entry.getValue());     counter++; } 

The output:

1: a - 3
2: b - 2

The explanation:

  1. You have your List<String[]> phrases first, you want to simply have an object that holds your Strings. Here a Stream<String> is a suitable object, because you only need to have a view on your phrases object, there is no point in actually storing the new data. You do this by calling phrases.stream().flatMap(Arrays::stream).
    1. This will first turn your List<String[]> in a Stream<String[]>.
    2. Then you use a method reference that describes the lambda stringArray -> Arrays.stream(stringArray) to acquire a Stream<String>.
    3. Then with the flatMap you add all elements of the resulting Stream<String> back into the original Stream<String>.
  2. Then you start at the getTopWords method, which returns a Stream<Map.Entry<String, Long>>, again to offer the flexibility to do what you want with the results, they are not stored yet at the point where it gets returned.
  3. First I added some error checking.
  4. Then I obtain a Comparator<Map.Entry<String, Long>> that will compare what the entry is with the lowest number of occurences. This is done by using a Comparator.comparingLong on the value of the entry, which is obtained with the method reference Map.Entry::getValue.
  5. Then I start the chain of operations on the input Stream<String>:
    1. First group the results by their identity, which normally produces a Map<String, List<String>>.
    2. The trick here is that I also used a downstream Collector, which counts the number of times the string occurs, hence it is called Collectors.counting().
    3. At this point I have a Map<String, Long> denoting the word and the number of occurences. It uses a Long, because this is what Collectors.counting() returns.
    4. Then I obtain a Set<Map.Entry<String, Long>> and convert it into a stream.
    5. Then I call sorted() on the Stream<Map.Entry<String, Long>> with the reversed comparator. This is done here, because type interference is not strong enough to use Comparator.comparingLong(Map.Entry::getValue).reversed().
    6. Then I limit() the stream by the top x elements.
  6. Now we have the Stream<Map.Entry<String, Long>>, and here we decide to collect it into a List<Map.Entry<String, Long>>.
  7. Here we continue with your old logic of having a counter attached to it.

A few points that are worth to note:

  1. The explicit comparator.reversed() is ugly, but neccessary to not result in type casting, which is even more ugly, it is a limitation of the current type interference. It might be only an issue in IDE's and the javac compiler might actually compile it though.
  2. The usage of Map.Entry<String, Long> is pretty bloated, but our most reasonable option, besides creating a Pair class ourselves and using Pair<String, Long>. This will hopefully be easier if Java 9 includes tuples (which logically include pairs) as more or less first-class citizens.
  3. In the whole method getTopWords we end up storing all the entries in memory once, with the Map<String, Long>, I am pretty sure there are ways around that, but not worth the effort here, only optimize this if it becomes a real bottleneck.
  4. I was hoping to use the Stream.forEach() method when processing the results, however this is not possible with the requirement that you want to have a counter. Again, more possibilities open up in Java 9 when we hopefully have BiStreams and tuples.

I hope this review has been helpful for you.

 
 
     
     
10
 
vote

En wordCount No necesita un 9988777665544331 . A TreeMap Pedidos Entradas por llaves, pero no lo necesita en absoluto. El propósito de este método es devolver un mapa de conteos de palabras, el ordenamiento de las entradas no importa. No es un error para usar un 9988777666544333 , no tiene sentido. Un HashMap habría sido mejor.

En printTopWords Usted está utilizando iteradores sin tipo. Eso no es una buena práctica, y los moldes dentro del bucle son feos. El bucle habría sido mejor así, usando el patrón del iterador:

  int wordsCounter = 1; for (Entry<String, Integer> entry : wordsMap) {     String key = entry.getKey();     int value = entry.getValue();     System.out.println(wordsCounter + ": " + key + " - " + value);     if (++wordsCounter > numberOfWords) {         break;     } }   

Su programa no separa bien las responsabilidades. No debe ordenar y imprimir en el mismo método. Sería mejor separar eso a 2 métodos, uno para ordenar y otro para imprimir. De esa manera, las pruebas unitarias también serán más fáciles, ya que sus casos de prueba podrían basarse en lo que devuelve el método de clasificación.

Un método algo más simple para ordenar por los valores habría estado usando un 99887776665544337 con un 9988777665544338 , por ejemplo:

  static class WordCountComparator implements Comparator<String> {     Map<String, Integer> base;     public WordCountComparator(Map<String, Integer> base) {         this.base = base;     }      public int compare(String a, String b) {         if (base.get(a) >= base.get(b)) {             return -1;         }         return 1;     } }  public static List<String> printTopWords(final int numberOfWords, List<String[]> phrases) {     Map<String, Integer> wordCountMap = wordCount(phrases);     Map<String, Integer> wordsSortedByCount = new TreeMap<String, Integer>(new WordCountComparator(wordCountMap));     wordsSortedByCount.putAll(wordCountMap);     // ... }   

Puede iterar sobre las entradas en TreeMap0 , están ordenados por el recuento de palabras.

Tenga en cuenta que no especificó el pedido de palabras que tienen la misma cuenta, por lo que el orden de esas no se especificará.

 

In wordCount you don't need a TreeMap. A TreeMap orders entries by keys, but you don't need it at all. The purpose of this method is to return a map of word counts, the ordering of entries doesn't matter. It's not an error to use a TreeMap, it's just pointless. A HashMap would have been better.

In printTopWords you are using iterators without type. That's not a good practice, and the casts inside the loop are ugly. The loop would have been better like this, using the iterator pattern:

int wordsCounter = 1; for (Entry<String, Integer> entry : wordsMap) {     String key = entry.getKey();     int value = entry.getValue();     System.out.println(wordsCounter + ": " + key + " - " + value);     if (++wordsCounter > numberOfWords) {         break;     } } 

Your program doesn't separate responsibilities well. You should not sort-and-print in the same method. It would be better to separate that to 2 methods, one to sort and another to print. That way unit testing will be easier too, as your test cases could be based on what the sorting method returns.

A somewhat simpler method to sort by values would have been using a Comparator with a TreeSet, for example:

static class WordCountComparator implements Comparator<String> {     Map<String, Integer> base;     public WordCountComparator(Map<String, Integer> base) {         this.base = base;     }      public int compare(String a, String b) {         if (base.get(a) >= base.get(b)) {             return -1;         }         return 1;     } }  public static List<String> printTopWords(final int numberOfWords, List<String[]> phrases) {     Map<String, Integer> wordCountMap = wordCount(phrases);     Map<String, Integer> wordsSortedByCount = new TreeMap<String, Integer>(new WordCountComparator(wordCountMap));     wordsSortedByCount.putAll(wordCountMap);     // ... } 

You could iterate over entries in wordsSortedByCount, they are sorted by the word count.

Keep in mind that you did not specify the ordering of words that have the same count, so the ordering of those will be unspecified.

 
 
   
   
7
 
vote

Algunas notas por ahora:

No está usando genéricos para su iterador, si usa

  TreeMap1  

y TreeMap2

No tendrás que escribir nada.


Puede usar un 99887766655443313 en lugar de un TreeMap4 para evitar tener que usar TreeMap5 . Mediante el uso de TreeMap6 podría llamar TreeMap7 en él para aumentar el valor.

 

A few notes for now:

You're not using generics for your iterator, if you use

Iterator<Entry<String, Integer>> entries = wordsMap.iterator(); 

and Entry<String, Integer> entry = entries.next();

you won't have to typecast anything.


You could use an AtomicInteger instead of an Integer to avoid having to use .put. By using AtomicInteger you could call .incrementAndGet() on it to increase the value.

 
 

Relacionados problema

6  Palindrome más largo en una matriz  ( Longest palindrome in an array ) 
Soy nuevo en la programación, y creo que este código podría mejorarse. ¿Alguna sugerencia? 'done'0 ...

5  Memoria / Performance of Merge Sort Code  ( Memory performance of merge sort code ) 
Escribí un código de tipo de combinación para un poco de bocadillo nocturno. Lo he puesto trabajando, pero solo estaba mirando a aprender si me faltaba algo e...

35  Demasiados bucles en la aplicación de dibujo  ( Too many loops in drawing app ) 
Tengo un método que tiene muchos bucles: #ifndef __RUNES_STRUCTURES_H #define __RUNES_STRUCTURES_H /* Runes structures. */ struct Game { char board[2...

1  Compruebe si dos cadenas son permutación entre sí  ( Check if two strings are permutation of each other ) 
private String sort(String word) { char[] content = word.toCharArray(); Arrays.sort(content); return new String(content); } private boolea...

8  Simple GCD Utility en Java  ( Simple gcd utility in java ) 
i anteriormente discutido El rendimiento se refiere a diferentes algoritmos GCD. Escribí una simple clase de Java que implementa el algoritmo binario GCD. E...

3  Generador de imágenes de Mandelbrot con iteración paralela  ( Mandelbrot image generator with parallel iteration ) 
Actualmente estoy tratando de optimizar esta clase que tengo para la generación fractal. La ecuación está destinada a ser conectable; He usado z => z*z + c ...

4  Simulación simple de red neural en C ++ (Ronda 2)  ( Simple neural network simulation in c round 2 ) 
Intro Ayer He publicado esta pregunta . Desde entonces, he actualizado mi código para incorporar estas sugerencias . También he eliminado la dependencia d...

2  IMACROS BOT para realizar refrescos  ( Imacros bot for performing refreshes ) 
Estoy tratando de simplificar este código. Parece que todo funciona como debería; Sin embargo, cuando en el bucle de actualización de Imacro, parece un poco i...

5  Encuentre el próximo número Prime - Control de flujo de los bucles anidados 'para `  ( Find the next prime number flow control of nested for loops ) 
Este código funciona perfectamente, pero me molesta. Tener un bucle etiquetado y anidado Bucle, con un Enumerable<T>.Empty()0 Declaración, y un 9988777665...

1  Integración de oscilador de fase perturbada  ( Perturbed phase oscillator integration ) 
Estoy integrando un sistema de osciladores de fase perturbados. Defino el sistema de ecuación y también la matriz jacobiana. Tengo que remodelar el vector dim...




© 2022 respuesta.top Reservados todos los derechos. Centro de preguntas y respuestas reservados todos los derechos