Take the 2-minute tour ×
Code Review Stack Exchange is a question and answer site for peer programmer code reviews. It's 100% free, no registration required.

I had a task where I was supposed to write a solution to sort entries(Name, score - in an array/list) in order of score and generate ranking based on higher score to lower scores.

I tried doing that, code is working what it was supposed to do but it seems very ambiguous and tightly coupled, I can't even write a unit test for it.

Could someone have a look and suggest.

I have pasted main method class, If anyone wants me to paste other relevent classes as well so do ask me please.

Any suggestion(s)/workaround(s) would be much appreciated.

public class JournalAnalyzer {
    final static int LENGTH_OF_JOURNAL = 5; //Length Journal array
    static int count = 0;
    static Integer ranking = 1;
    static Journal[] journals = new Journal[LENGTH_OF_JOURNAL];
    static ArrayList<Journal> list = new ArrayList<Journal>();
    static Object[] scoreList = new Object[LENGTH_OF_JOURNAL];
    static String journalName;
    static Double journalScore;
    static Integer journalRank;
    static Boolean journalReview;

private static void initData(Journal[] journals) {
    journals[0] = new Journal(0, "Journal A", 5.6, false);
    journals[1] = new Journal(0, "Journal B", 2.6, false);
    journals[2] = new Journal(0, "Journal C", 3.2, false);
    journals[3] = new Journal(0, "Journal D", 4.1, false);
    journals[4] = new Journal(0, "Journal E", 1.6, false);
}

private static int fillJournalList(int count, Journal[] journals,
        int LENGTH_OF_JOURNAL, ArrayList<Journal> list) {

    for (int i = 0; i < LENGTH_OF_JOURNAL; i++) {
        journalRank = new Integer(journals[i].getRank());
        journalName = new String(journals[i].getJournal());
        journalScore = new Double(journals[i].getScore());
        journalReview = new Boolean(journals[i].getIsReviewed());

        if (!journalReview) {
            Journal value = new Journal(journalRank, journalName,
                    journalScore, journalReview);
            list.add(value);
            count++;
        } else {
            continue;
        }
    }
    return count;
}

private static void GenerateRanking(Integer ranking, int count,
        ArrayList<Journal> list, Object[] scoreList) {
    Double scoreComparatorOne;
    Double scoreComparatorTwo;

    Comparator<Journal> scores = new SortScore();
    Collections.sort(list, scores);

    for (int i = 0; i < count; i++) {
        scoreComparatorOne = (list.get(i)).getScore();
        if (i == 0) {
            scoreComparatorTwo = (list.get(i)).getScore();
        } else {
            scoreComparatorTwo = (list.get(i - 1)).getScore();
        }
        if (scoreComparatorOne.equals(scoreComparatorTwo)) {
            if (ranking == 0) {
                ranking += 1;
            } else {
                ranking = ranking;
            }
        } else {
            ranking = i + 1;
        }

        (list.get(i)).setRank(ranking);
        scoreList[i] = (list.get(i));

    }
}

private static void displayJournal(int counter, Object[] scoreList) {
    JournalOutput printer = new JournalOutput();
    printer.display(count, scoreList);
    System.exit(0);
}


public static void main(String[] args) throws IOException {
    initData(journals);
    count = fillJournalList(count, journals, LENGTH_OF_JOURNAL, list);
    GenerateRanking(ranking, count, list, scoreList);
    displayJournal(count, scoreList);
}
}

My question is how to minimize and simplify this code so that I can easily write a unit test for this class

share|improve this question

migrated from stackoverflow.com Nov 30 '12 at 11:21

This question came from our site for professional and enthusiast programmers.

1  
What is your question? –  Code-Guru Nov 29 '12 at 23:45
    
My question is how to minimize and simplify this code so that I can easily write a unit test for this class. –  Ali Jay Nov 29 '12 at 23:52
add comment

1 Answer 1

up vote 4 down vote accepted

You could change this:

for (int i = 0; i < count; i++) {
        scoreComparatorOne = (list.get(i)).getScore();
        if (i == 0) {
            scoreComparatorTwo = (list.get(i)).getScore();
        } else {
            scoreComparatorTwo = (list.get(i - 1)).getScore();
        }
        if (scoreComparatorOne.equals(scoreComparatorTwo)) {
            if (ranking == 0) {
                ranking += 1;
            } else {
                ranking = ranking;
            }
        } else {
            ranking = i + 1;
        }
    (list.get(i)).setRank(ranking);
    scoreList[i] = (list.get(i));
 }

Theres no need to verify all the time this conditional:

if (i == 0) {
                scoreComparatorTwo = (list.get(i)).getScore();
            } else {
                scoreComparatorTwo = (list.get(i - 1)).getScore();
            }

inside the loop. Since i it will be equal to 0 only once.

Modifying to:

...
Double scoreComparatorOne = (list.get(0)).getScore();
Double scoreComparatorTwo = (list.get(0)).getScore();
ranking = getRating(scoreComparatorOne,scoreComparatorTwo,ranking,0);
(list.get(0)).setRank(ranking);
scoreList[0] = (list.get(0));

for (int i = 1; i < count; i++){
        scoreComparatorOne = (list.get(i)).getScore();
        scoreComparatorTwo = (list.get(i - 1)).getScore();
        ranking = getRating(scoreComparatorOne,scoreComparatorTwo,ranking,i);
        (list.get(i)).setRank(ranking);
        scoreList[i] = (list.get(i));
}

where getRating is:

 public Integer getRating (Double sCOne, Double sCTwo, Integer ranking, int i)
    {
           if(sCOne.equals(sCTwo))
              return (ranking == 0) ? 1 : ranking;
           else return i + 1;
    }

also you can turn your fillJournalList into (but this is not a major problem):

private static int fillJournalList(Journal[] journals,
    int LENGTH_OF_JOURNAL, ArrayList<Journal> list) {

for (int i = 0; i < LENGTH_OF_JOURNAL; i++){

    if (!journals[i].getIsReviewed())
        list.add(new Journal(new Integer(journals[i].getRank(),                  
                              new String(journals[i].getJournal(),
                              new Double(journals[i].getScore(), 
                              false);

return list.size();

}

This way you only create the new Objects when you will insert them. Not in every for iteration. With the above modification there no need to this:

public class JournalAnalyzer {
    ...
    static String journalName;
    static Double journalScore;
    static Integer journalRank;
    static Boolean journalReview;

You can remove all four variables.

share|improve this answer
    
Thank you very much indeed. –  Ali Jay Nov 30 '12 at 9:21
    
Hi, I made changes to the code, now its throwing NullPointerException. Due to scoreList skipping to fill scorelist[0] element. –  Ali Jay Nov 30 '12 at 9:52
    
@AliJay did you manage to solve? –  dreamcrash Nov 30 '12 at 12:13
    
@@dreamcrash I have made changes like mentioned in my last comment, now its throwing NullPointerException. Due to scoreList skipping to fill scorelist[0] element. And it is due to GenerateRanking method where for loop starts from counter 1. –  Ali Jay Nov 30 '12 at 12:17
    
post your code here codepaste.net and post it back the link here so i can see it –  dreamcrash Nov 30 '12 at 12:21
show 15 more comments

Your Answer

 
discard

By posting your answer, you agree to the privacy policy and terms of service.

Not the answer you're looking for? Browse other questions tagged or ask your own question.