0
\$\begingroup\$

This question is a follow-up of this question. The aim of the questions is to find sum of number of times that each character of source occurs in the target.

In the new code, I've renamed the char array and added the functionality to prevent the following "if a character is duplicated in the source, you will double-count it in the result."

I would like to know if there is still a better solution for it.

It was mentioned in one of the answers of the previous question that I should use HashMap or other data structures for chars more than 100, but I am not sure how and why.

String target = "alex alexander";
String source = "ardx"; 
Map<Character,Integer> tempMap = new HashMap<>();

char[] tchars = source.toCharArray();

int occur = 0;
int occurrence = 0;

for (int i = 0; i < tchars.length; i++) {
   if(!tempMap.containsKey(tchars[i])){ //if the occurrence of the current char 
        String temp = target;           //   has been already calculated  
        int index = temp.indexOf(tchars[i]);

        while (index != -1) {
            occur++;
            index = temp.indexOf(tchars[i], index + 1);
        }
        tempMap.put(tchars[i],occur);
        occurrence += occur; //add to the total counter
        occur = 0;           //reset the single counter
   }else{
       occurrence += tempMap.get(tchars[i]);
   }

}
System.out.println(occurrence);

Output

a is added to the map and its associated counter is 3
r is added to the map and its associated counter is 1
d is added to the map and its associated counter is 1
x is added to the map and its associated counter is 2
25
\$\endgroup\$
2
\$\begingroup\$

Sorry, but this looks worse. I guess, you've been confused by the different ideas presented in the answers. And I am, confused, too. I don't understand it, so just some minor comments:

//reset the single counter

This is a sign that you've defined the counter in a wrong scope. Define it inside of the loop.


tempMap, temp... everything is temporary, we too. But this doesn't mean that it can't be properly named. Or removed... you need no temp String anymore.


I think I sort of understand it. The tempMap keeps track of the number of occurrences per character. So name it like it....

I don't think it really works. However, why not use a Set to keep track of what chars you've seen already? The logic is then trivial: "already seen -> skip it".

Enough for now.

\$\endgroup\$
  • \$\begingroup\$ Thanks, the reason that I used //reset the single counter is that once the counter is added to the total counter I will reset it, so it should not be in the loop. Regarding the naming I will rename them, regarding the set, I used map to keep counter of each character, so if a character is repeated its associated counter will be added to the total counter, I am about to write an example in the question for clarification. \$\endgroup\$ – Jack Sep 17 '14 at 5:10
  • \$\begingroup\$ @Jack Is it possible for occur to be non-zero before the while loop? If no, it's the place where it should be declared. If yes, please show how. \$\endgroup\$ – maaartinus Sep 17 '14 at 5:29
  • \$\begingroup\$ @maaartinus I do not get what you mean by the location of occur. It is declared outside of the loops to avoid re-declaring it in every step of the loop. It needs to be set to zero to get reset before being used in the next steps of the loop. \$\endgroup\$ – Jack Sep 17 '14 at 5:38
  • 1
    \$\begingroup\$ But re-declaring it is not to be avoided, that's how it always should be. \$\endgroup\$ – maaartinus Sep 17 '14 at 5:41
1
\$\begingroup\$

ahem. edited out where I wrongly pointed out that new HashMap<>() was incorrect, eventhough it actually is ;)

As was pointed out, temp is a name you may want to avoid unless you are swapping members and you are using a temporary storage.

your occur only is used inside this method so it can be situated inside that scope.

 if(!tempMap.containsKey(tchars[i])){
     int occur = 0;

this means you do not have to set it to 0 again. Inside a specific scope, it's not an expense to have another stack variable. More so if it gets released quickly because you exit the scope.

this line is unnecessary:

String temp = target;

You never overwrite target or temp afterwards, so you simply can use target. One thing that you could use a local variable for though it tchars[i]. you use that a lot and it refers to the same character the whole time.

char currentCharacter = tchars[i]; 

will mean you can talk about 'currentCharacter' instead which at least is descriptive.

even easier, since all you do is loop over the characters in tchars, you can do:

for (char currentCharacter : tchars) {

}

You don't use i for anything else, so now you don't even need it anymore.

not everyone likes assignment inside a while, but this can be made simpler:

int index = temp.indexOf(tchars[i]);
while (index != -1) {
    occur++;
    index = temp.indexOf(tchars[i], index + 1);
}

it can be:

int index = -1;
while ( (index = target.indexOf(tchars[i], index + 1)) != -1) {
    occur++;
} 

Now you don't have 2 types of indexOf floating around which will have to be kept in synch. I know, you aren't gaining much from it but it's generally better not to have 2 versions of the same operation. Once might get refactored and the other might be left in there. anyways I admit, this is not the strongest suggestion I am making.

In any case, I think your occurance is miscalculating still, if source contains a character twice, then you will still add the number of that character 2 times. This could be intentional ofcourse.

Assuming you only count each unique character from source in target, I would use 2 loops to generate this.

    String target = "alex alexander";
    String source = "ardx";
    Map<Character, Integer> characterCount = new HashMap<Character, Integer>();

    for (char sourceCharacter : source.toCharArray()) {
        characterCount.put(sourceCharacter, 0);
    }

and then do your counting by going over each char in target. if it is a character we are counting then the char exists as key in the map. If it's not a character we count, we skip to the next character.

    int totalCount = 0;

    for (char targetCharacter : target.toCharArray()) {
        if (!characterCount.containsKey(targetCharacter)) {
            continue; // not a character we are counting.
        }

        // can be done in 1 statement ofcourse
        int currentCount = characterCount.get(targetCharacter);
        characterCount.put(targetCharacter, currentCount + 1);

        totalCount++;
    }
    System.out.println(totalCount);

This way you don't need to run through the target output twice (once to see if it is a character to count, and then to do the actual counting)

I'm not a great fan of doing totalCount++ in the main loop. I'd rather use a 3rd loop that would total up all the counts. This way if the second loop (the counting loop) ever changes, you don't mess up the totalling up.

so something like:

int totalCount = 0;
for (int currentCount : characterCount.values()) {
    totalCount += currentCount;
}
System.out.println(totalCount);

it's another looping but the loop is fast enough and if you ever want to move the 'counting' loop into it's own method, you don't need worry about the total.

\$\endgroup\$
  • \$\begingroup\$ I'm hoping this is a copy-paste error but it should really be: new HashMap(); No it's not a copy pasted error see this for more info : stackoverflow.com/a/4167148 \$\endgroup\$ – Marc-Andre Oct 20 '14 at 18:51
  • \$\begingroup\$ heh shows I haven't used java 1.7 much :) thanksf or pointing it out. \$\endgroup\$ – Joeblade Oct 20 '14 at 22:04

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.