HiveBrain v1.2.0
Get Started
← Back to all entries
patternjavaMinor

Performance for tags for cars in Java

Submitted by: @import:stackexchange-codereview··
0
Viewed 0 times
codereviewjavastackoverflow
javatagscarsforperformance

Problem

private static boolean trovato , ok = false;

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

HashMap> map = new HashMap>();
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("")) {

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

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

for (int i = 0  ; i  hs = new HashSet () ; 
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.

Solution

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 ` marker, which didn't have its own variable and it deserves to have one:

public static final String END_MARKER = "";


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> map = new HashMap>();
// ...
map = new HashMap>();
// ...
HashSet hs = new HashSet();


Do like this:

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


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 hs = new HashSet();
    map.put(stringa, hs);
    map.get(stringa).add(elemento);
}


You can simplify:

stringa += letteraPerLettera[i];
if (!map.containsKey(stringa)) {
    Set hs = new HashSet();
    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 hs = map.get(stringa);
if (hs == null) {
    hs = new HashSet();
    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  cars = map.get(tagPrefix);
        if (cars == null) {
            cars = new HashSet();
            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)`)

Code Snippets

public static final String ADD_COMMAND = "a";
public static final String ITEM_COMMAND = "i";
public static final String SEARCH_COMMAND = "s";
public static final String END_MARKER = "<END>";
HashMap<String, HashSet<String>> map = new HashMap<String, HashSet<String>>();
// ...
map = new HashMap<String, HashSet<String>>();
// ...
HashSet<String> hs = new HashSet<String>();
Map<String, Set<String>> map = new HashMap<String, Set<String>>();
// ...
map = new HashMap<String, Set<String>>();
// ...
Set<String> hs = new HashSet<String>();
BufferedReader stdin = new BufferedReader(new InputStreamReader(System.in));

Context

StackExchange Code Review Q#62346, answer score: 7

Revisions (0)

No revisions yet.