5
\$\begingroup\$
private static boolean trovato , ok = false;

public static void main(String args[]) throws IOException {

HashMap<String, HashSet<String>> map = new HashMap<String, HashSet<String>>();
BufferedReader stdin = new BufferedReader(new InputStreamReader(System.in));

String line;
line = stdin.readLine();
String a = "a" ;
String ii = "i" ;
String s = "s" ;
int cont = 0 ;

while (!line.equals("<END>")) {

String stringa = "";
String elemento = "" ;


if (line.startsWith(ii)) {
map = new HashMap<String, HashSet<String>>();
}
else if (line.startsWith(a)) {
String[] letteraPerLettera = line.split("");

for (int i = 0  ; i < letteraPerLettera.length - 2; i++) {

if (!letteraPerLettera[i].equals(" ")) {

if (ok)
{
element = line.substring(2,i-2) ;

trovato  = true ;
ok = false ;
}

if (trovato )
{   
stringa+=letteraPerLettera[i];
if (map.containsKey(stringa)) {
map.get(stringa).add(elemento);
} else 
{
HashSet <String> hs = new HashSet<String> () ; 
map.put(stringa, hs );
map.get(stringa).add(elemento);
}
}

}




else {



cont ++ ;


if (cont == 2)


ok = true ;



stringa = "";

}

}


}


else if (line.startsWith(s)) {
String subtag = line.substring(2,line.length()-3);
if (map.containsKey(subtag)) {


System.out.println(map.get(subtag).size());
} else {
System.out.println("missing");
}
}


cont = 0 ; 
trovato = false ;
line = stdin.readLine();
}
stdin.close();
}

My code has an input like this:

i3
a Duna automobile Deserto -1 
a Nissan auto automobile -1 
s aut -1

i2
a Pesca Sport Frutta -1
s sport -1

END
  • i = 3 means that there are 3 lines.
  • "a" means that I'm adding an element (first occurrence after "a") and the other words after the element are my "tags".
  • "s" means that I have to find the tags that have as substring the word "aut" for the first block and "sport" for the second block.

At the end the output is like this:

2 
1

"2" means I have two elements "duna" & "nissan" that have tags (auto, automobile, auto) that start with the word "aut". "1" means I have the element "pesca" that has tag (sport) that starts with the sub-tag "sport".

With END, the program is finished.

\$\endgroup\$
7
\$\begingroup\$

Variables, scope, naming

Since you have only one method (main), there's no need to declare trovato and ok outside of it. Move them inside main, and make them local variables.

On the other hand, the variables a, ii, and s are for constant strings, indicating special commands in your command line interface, so it would be good to make them constants, loud and clear at the top of the class, with meaningful names, for example:

public static final String ADD_COMMAND = "a";
public static final String ITEM_COMMAND = "i";
public static final String SEARCH_COMMAND = "s";

The same goes for the <END> marker, which didn't have its own variable and it deserves to have one:

public static final String END_MARKER = "<END>";

Your program becomes instantly more readable. These constants are now also protected from mistakes: you cannot accidentally reassign their values, as they are final.

Furthermore, the variables trovato, cont and ok are actually only used when you're processing ADD_COMMAND. You should declare and initialize them there. This will also make it clear and easy to see that you don't need to reset their values at the end of the loop.

Good coding practices

Use interface types when declaring variables (and function parameters). Instead of this:

HashMap<String, HashSet<String>> map = new HashMap<String, HashSet<String>>();
// ...
map = new HashMap<String, HashSet<String>>();
// ...
HashSet<String> hs = new HashSet<String>();

Do like this:

Map<String, Set<String>> map = new HashMap<String, Set<String>>();
// ...
map = new HashMap<String, Set<String>>();
// ...
Set<String> hs = new HashSet<String>();

And try to think of better names for your maps and sets instead of map and hs. For example cars and tags.


BufferedReader stdin = new BufferedReader(new InputStreamReader(System.in));

Calling this BufferedReader as stdin is confusing and it invites mistakes. stdin is really System.in itself. A better name would be reader, or bufferedReader.


Always use braces with if statements. Instead of:

if (cont == 2) 
    ok = true;

Do like this:

if (cont == 2) {
    ok = true;
}

Simplify

Notice the duplicated line in this code:

stringa += letteraPerLettera[i];
if (map.containsKey(stringa)) {
    map.get(stringa).add(elemento);
} else {
    Set<String> hs = new HashSet<String>();
    map.put(stringa, hs);
    map.get(stringa).add(elemento);
}

You can simplify:

stringa += letteraPerLettera[i];
if (!map.containsKey(stringa)) {
    Set<String> hs = new HashSet<String>();
    map.put(stringa, hs);
}
map.get(stringa).add(elemento);

This is not very efficient though, because you search for stringa twice: first with containsKey and then again with get. This is equivalent and more efficient:

Set<String> hs = map.get(stringa);
if (hs == null) {
    hs = new HashSet<String>();
    map.put(stringa, hs);
}
hs.add(elemento);

Last words...

Even with the above improvements, I find this code almost unreadable. They way you process the lines, it's just far more complicated than it needs to be. For example, instead of processing the ADD_COMMAND letter by letter, it would make more sense, and more readable to process word by word, like this:

String[] parts = line.split(" ");

// 0: the command, 1: the car name, 2, ...: tags, length-1: -1 (???)
for (int i = 2; i < parts.length - 1; ++i) {
    String car = parts[1];
    String tag = parts[i];
    for (int j = 1; j <= parts[i].length(); ++j) {
        String tagPrefix = tag.substring(0, j);
        Set<String> cars = map.get(tagPrefix);
        if (cars == null) {
            cars = new HashSet<String>();
            map.put(tagPrefix, cars);
        }
        cars.add(car);
    }
}

This is almost the same as the original program, except it also fixes a bug: the original program drops the last letter of car names (resulting in "Dun", and "Nissa"), this one doesn't.

On a related note, do you really need the -1 at the end of lines? It would be easy to make it work without them.

Instead of using a BufferedInputReader, a Scanner can be easier to work with. I recommend to take a look at it (hint: Scanner scanner = new Scanner(System.in))

\$\endgroup\$
  • \$\begingroup\$ Thank you very much.How Could I use Thread to make more efficient my program ? \$\endgroup\$ – user3572461 Sep 9 '14 at 8:25
  • 3
    \$\begingroup\$ There's absolutely no need whatsoever to use threads in this program. Don't do it. \$\endgroup\$ – Stop ongoing harm to Monica Sep 9 '14 at 8:40
  • \$\begingroup\$ I would that threads according to my n-core make insert operation.Only (map.put) \$\endgroup\$ – user3572461 Sep 9 '14 at 8:46
  • \$\begingroup\$ The current program doesn't have performance issues. Using threads to improve the performance would be premature optimization and overkill. There are more important things you can improve, for example make it testable, add some unit tests, simplify its main flow. \$\endgroup\$ – Stop ongoing harm to Monica Sep 9 '14 at 9:03
  • 3
    \$\begingroup\$ Have you tried my suggested rewrite for processing ADD_COMMAND? It might help. Also, given the sample and description you gave, the car names are not used. If that's the case, then instead of a Map<String, Set<String>>, you could use a Map<String, Integer> which can help. Another micro optimization that can help is clearing the hash map instead of recreating. I find it highly unlikely that threads will be the answer, and you haven't given a sample where the benefits of using threads would be measurable. \$\endgroup\$ – Stop ongoing harm to Monica Sep 9 '14 at 9:29
6
\$\begingroup\$

The single most important thing you can do right now is indent your code so that it can be read. It's nearly incomprehensible as it is. You should also be consistent with where you put the braces. Sometimes you put them at the end of the line, sometimes at the beginning. I believe it's Java convention to place them at the end.

while (!line.equals("<END>")) {

    String stringa = "";
    String elemento = "" ;


    if (line.startsWith(ii)) {
        map = new HashMap<String, HashSet<String>>();
    }
    else if (line.startsWith(a)) {
        String[] letteraPerLettera = line.split("");

        for (int i = 0  ; i < letteraPerLettera.length - 2; i++) {

            if (!letteraPerLettera[i].equals(" ")) {

            if (ok) {
                element = line.substring(2,i-2) ;

                trovato  = true ;
                ok = false ;
            }

            if (trovato ) { 
                stringa+=letteraPerLettera[i];

                if (map.containsKey(stringa)) {
                    map.get(stringa).add(elemento);
                } else {
                    HashSet <String> hs = new HashSet<String> () ; 
                    map.put(stringa, hs );
                    map.get(stringa).add(elemento);
                }
            }              
        }
\$\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.