4
\$\begingroup\$

This is a baseline tagger coed I have written. How can I optimize this code?

public class Tagger {

private ArrayList<List> copyTagsList = new ArrayList<List>();

public Tagger(WordChecker tagsList){
    for(int i =0; i<tagsList.getCheckedTags().size();i++){
    copyTagsList.add(i,tagsList.getCheckedTags().get(i));
    copyTagsList.get(i).set(1,0);
    }
}

public void extractTag(String word, ArrayList<ArrayList> trainList){

int freq = 0;
ArrayList<List> tagFreqOfWord = new ArrayList<List>();
System.out.println(word);

for(ArrayList wordState: trainList)
{           
    if (word.equals(wordState.get(1)))
    {

        freq+=1;
        if (tagFreqOfWord.size() == 0)
        {
            for (int i = 0 ; i<copyTagsList.size();i++)
            {
                if (((String)wordState.get(4)).equalsIgnoreCase((String)copyTagsList.get(i).get(0)))
                {
                    tagFreqOfWord.add(copyTagsList.get(i));
                    tagFreqOfWord.get(0).set(1, (int)1);
                    break;
                }
            }
        }
        else 
        {
            Boolean exist= false;
            for (List j :tagFreqOfWord)
            {
                if (((String)wordState.get(4)).equalsIgnoreCase((String)j.get(0)))
                {
                    int var = (int)tagFreqOfWord.get(tagFreqOfWord.indexOf(j)).get(1);
                    var+=1;
                    tagFreqOfWord.get(tagFreqOfWord.indexOf(j)).set(1,var);
                    exist = true;
                    break;
                }
            }           
            if (!exist)
            {
                List newTag = new ArrayList();
                newTag.add(wordState.get(4));
                newTag.add(1, 1);;
                tagFreqOfWord.add(newTag);
            }                       
        }
    }
}
System.out.println(tagFreqOfWord);  
}           

}

Confusion matrix (Part of AccuracyChecker class):

public ArrayList<List> createConfusionMatrix(WordChecker checkedTagsList,    ArrayList<ArrayList> listOfWords){
//BeanComparator fieldComparator = new BeanComparator();
//Collections.sort(checkedTagsList, fieldComparator);

int distinctTagsSize = correctTags.size();
for(int i=0; i <distinctTagsSize;i++){
    List<Integer> list = new ArrayList<Integer>();
    confusionMatrix.add(i, list);;
    for(int j=0;j<distinctTagsSize; j++ ){
        confusionMatrix.get(i).add(j, 0);
        if (i == j)
            confusionMatrix.get(i).set(j, checkedTagsList.getCheckedTags().get(i).get(1));
    }
}

for(int i=0; i <distinctTagsSize;i++){
    for(int j=0;j<listOfWords.size();j++){
            if (((String)correctTags.get(i).get(0)).equalsIgnoreCase((String)listOfWords.get(j).get(4))){
                if(!((String)listOfWords.get(j).get(5)).equalsIgnoreCase((String)listOfWords.get(j).get(4))){
                    for (List m: correctTags){
                        if(((String)listOfWords.get(j).get(5)).equalsIgnoreCase((String)m.get(0))){
                            int freq= (int)confusionMatrix.get(i).get(correctTags.indexOf(m));
                            freq+=1;
                            confusionMatrix.get(i).set(correctTags.indexOf(m),freq);
                        }
                    }
                }

        }
    }
}
return confusionMatrix;
}

Do you think using 2D ArrayList is better?

\$\endgroup\$
5
\$\begingroup\$
private ArrayList<List> copyTagsList = new ArrayList<List>();

You should always use generic version (use raw type only if you have a good reason!)

Same inside extractTag using generic version you will not need anymore String cast.

The same will be for other collection.

I usually tend to cache the .size() specially when using it in a for to avoid method call overhead, but i don't remember if JVM could optimizate the call.

Example here:

for (int i = 0 ; i<copyTagsList.size();i++)

could be

int size = copyTagsList.size();
for (int i = 0 ; i<size;i++)

Avoid Wrapper objects if not needed (only in generic!)

tagFreqOfWord.get(0).set(1, (int)1);

No need for cast here.

Boolean exist= false;

Here Boolean refer to the wrapper class, not to the native type.. with this line you cause a boxing-unboxing everytime. Replace it with boolean exist = false;.

Use better names

for (List j :tagFreqOfWord)

while reading for (int i = 0; i < x; i++) i is OK in most cases, reading this line could lead to confusion because someone could think: "well, it's j it could be the second index in a inner-for!"

Or, it could lead to confusion yourself, use one-char-name only if you are working with indexs in for/while/etc. (outside you could use idx!)

int var = (int)tagFreqOfWord.get(tagFreqOfWord.indexOf(j)).get(1);

Do you really need the cast here? I don't think. If it's a Integer it will be read as int, so cast needed.

int var = tagFreqOfWord.get(tagFreqOfWord.indexOf(j)).get(1);
var+=1;
tagFreqOfWord.get(tagFreqOfWord.indexOf(j)).set(1,var);

You do indexOf() two times, which could be a problem (it checks two times the same thing) just do it one time and save the result in a variable.

int indexOf = tagFreqOfWord.indexOf(j);
int var = tagFreqOfWord.get(indexOf).get(1);
var+=1;
tagFreqOfWord.get(indexOf).set(1,var);

Remember, indexOf:

Returns the index of the first occurrence of the specified element in this list, or -1 if this list does not contain the element. More formally, returns the lowest index i such that (o==null ? get(i)==null : o.equals(get(i))), or -1 if there is no such index.

You chould check if it returns -1

Seems like wordState.get(4) can be saved and avoid .get() call N times (In a LinkedList, call .get everytime could be a problem)

So it could be

Boolean exist= false;
String specialWord = (String)wordState.get(4);
for (List j :tagFreqOfWord)
{
    if (specialWord.equalsIgnoreCase((String)j.get(0)))
    {
        int var = (int)tagFreqOfWord.get(tagFreqOfWord.indexOf(j)).get(1);
        var+=1;
        tagFreqOfWord.get(tagFreqOfWord.indexOf(j)).set(1,var);
        exist = true;
        break;
    }
}

if (!exist)
{
    List newTag = new ArrayList();
    newTag.add(specialWord);
    newTag.add(1, 1);;
    tagFreqOfWord.add(newTag);
}

So you avoid do a Cast everytime!


int distinctTagsSize = correctTags.size();
for(int i=0; i <distinctTagsSize;i++){
    List<Integer> list = new ArrayList<Integer>();
    confusionMatrix.add(i, list);;
    for(int j=0;j<distinctTagsSize; j++ ){
        confusionMatrix.get(i).add(j, 0);
        if (i == j)
            confusionMatrix.get(i).set(j, checkedTagsList.getCheckedTags().get(i).get(1));
    }
}

Here you create a new list everytime, it's what you want? Ok i know it's Code review so you already know the code is correct, i just want to be sure you know what happens here.

For now i can't see anything wrong in this block.

Here

for(int i=0; i <distinctTagsSize;i++){
    for(int j=0;j<listOfWords.size();j++){
            if (((String)correctTags.get(i).get(0)).equalsIgnoreCase((String)listOfWords.get(j).get(4))){
                if(!((String)listOfWords.get(j).get(5)).equalsIgnoreCase((String)listOfWords.get(j).get(4))){
                    for (List m: correctTags){
                        if(((String)listOfWords.get(j).get(5)).equalsIgnoreCase((String)m.get(0))){
                            int freq= (int)confusionMatrix.get(i).get(correctTags.indexOf(m));
                            freq+=1;
                            confusionMatrix.get(i).set(correctTags.indexOf(m),freq);
                        }
                    }
                }

        }
    }
}

(String)correctTags.get(i).get(0) will not change while for(..; j; ..) so you could do

for(int i=0; i <distinctTagsSize;i++){
   String cache1 = (String)correctTags.get(i).get(0);
    for(int j=0;j<listOfWords.size();j++){

and then use cache1. (example name!!!!)

You could do the same thing for (String)listOfWords.get(j).get(4) and (String)listOfWords.get(j).get(5) but inside for (..; j; ..) scope.

Again, as above

int freq= (int)confusionMatrix.get(i).get(correctTags.indexOf(m));

You don't need a cast here, and you can save correctTags.indexOf(m) return value inside a variable and reuse it inside

confusionMatrix.get(i).set(correctTags.indexOf(m),freq);

too

confusionMatrix.get(i).set(indexOfSaved, freq);

I hope my english is clear, i will try to fix grammar errors..

Anyway maybe i missed something.

\$\endgroup\$
  • \$\begingroup\$ Thanks so much, I will go through all your comments and check the performance. but right now I can see how much better my code will be. Than you again for sparing so much time and explaining each part. \$\endgroup\$ – msc87 Mar 29 '14 at 14:33
  • \$\begingroup\$ Marco:"int var = (int)tagFreqOfWord.get(tagFreqOfWord.indexOf(j)).get(1); Do you really need the cast here? I don't think. If it's a Integer it will be read as int, so cast needed." without casting, there would be an object which can' be converted to int. the right-hand side of equation is an object. in my lists mostly I have both strings and integers which determine the frequency of that word or tag. so I used nested lists (ArrayList) with this hope that I can store any kind of object. Do you have any better idea? \$\endgroup\$ – msc87 Mar 29 '14 at 14:42
  • \$\begingroup\$ You should avoid this kind of design, why you want to store "any kind of object" in an ArrayList? It could lead to bugs and ClassCastException. \$\endgroup\$ – Marco Acierno Mar 29 '14 at 14:52
  • \$\begingroup\$ If you have an ArrayList of words use ArrayList<String> then the ArrayList of frequency will be ArrayList<Integer> \$\endgroup\$ – Marco Acierno Mar 29 '14 at 14:52
  • 1
    \$\begingroup\$ Why not a class at this point? class Word { private String word; private int frequency; public void increment(); } and call increment() to increment, getFreq() to get. So you don't use two lists but only one \$\endgroup\$ – Marco Acierno Mar 29 '14 at 15:01

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.