Was failing 2 & 5, now failing 2 & 4

Question about the task

Under discussion

I had it initially where it was returning the right answer, but I didn't write my code to utilize threads (I didn't have a start and run method). I changed some things around while I thought I was keeping my logic the same, and now not only did I fail to create and start a ReadThread, now I'm not even getting the right answer. I feel like my code starts off nice and neat and simple, but as I struggle to reach validation I start hobbling things on to pass requirements and everything just starts looking like a mess. Basically I was using a comparator to sort the values in my HashMap, and then, because the last value was the largest, I just cycled through replacing the result variable every time, as the last getKey would be the Key associated with the largest frequency of occurring byte. Maybe that was stupid, but it worked initially, and now it doesn't. And apparently I'm not even starting a thread?

Read file names from the console until the word "exit" is entered.
Pass the file name to the ReadThread thread.
The ReadThread thread should find the byte that occurs most frequently in the file, and add it to resultMap,
where the String parameter is the file name and the Integer parameter is the relevant byte.
Close the streams.

Requirements:

The program must read file names from the console until the word "exit" is entered.
For each file, create and start a ReadThread thread.
After each thread is start, ReadThread should create its own file input stream.
Then each thread must find the most frequently occurring byte in its file and add it to resultMap.
The file input stream in each thread must be closed.

package com.codegym.task.task18.task1823; import java.io.BufferedInputStream; import java.io.FileInputStream; import java.util.*; /* Threads and bytes */ public class Solution { public static Map<String, Integer> resultMap = new HashMap<String, Integer>(); public static void main(String[] args) throws Exception{ String file = null; Scanner sc = new Scanner(System.in); while (true) { file = sc.nextLine(); if (file == "exit") break; else { ReadThread t = new ReadThread(file); t.start(); resultMap.put(file, t.frequent); } } } public static class ReadThread extends Thread { FileInputStream fis; BufferedInputStream bis; Integer frequent; public ReadThread(String fileName) throws Exception{ this.fis = new FileInputStream(fileName); this.bis = new BufferedInputStream(fis); } public void run() { try { this.frequent = mostFrequent(this); } catch (Exception e) {} } public static Integer mostFrequent (ReadThread thread) throws Exception { HashMap <Integer, Integer> count = new HashMap<>(); int i; while ((i = thread.bis.read()) != -1) { count.put(i, count.getOrDefault(i, 0) + 1); } thread.bis.close(); List<Map.Entry<Integer, Integer> > list = new LinkedList<Map.Entry<Integer, Integer>>(count.entrySet()); Collections.sort(list, new Comparator<Map.Entry<Integer, Integer>>() { @Override public int compare(Map.Entry<Integer, Integer> o1, Map.Entry<Integer, Integer> o2) { return o1.getValue().compareTo(o2.getValue()); } }); Integer result = 0; for (Map.Entry<Integer, Integer> aa : list) { result = aa.getKey(); } return result; } } }

Comments (5)

Popular
New
Old

You must be signed in to leave a comment

Guadalupe Gagnon

Level 37 , Tampa, United States

16 May 2021, 03:19

I think the problem is going to be line 20. Make sure to compare Strings with the .equals() method not with the equality operator: == *tips* -you don't need an else block at line 22. If the if statement is true then the loop will break and the code in the "else" block won't be reached regardless. Technically there is no negative effects in performance of doing it the way it is, but it does make code easier to read when it isn't indented over, more so when it starts being 4, 5, or more indents. -Because you are sorting the list by the Entry value, you can grab the last entry in the list to get the correct result instead of iterating through the whole map 1 at a time until the last one is reached. Furthermore, because it is a linked list, you can sort it more efficiently in reverse order (return o2 compared to o1 instead), and then just set result to the entry.value at index 0. - the 'this' keyword is only needed when passing the current object as a return value or to a method; or there is a local variable name that conflicts with a class variable name, say if you save "fileName" as a class variable, in the constructor you would need to use "this.fileName = fileName" or the compiler will be confused. In the above code you can remove all the keywords that don't fit that criteria: lines 36, 37 and the first one on line 41. Once again, this is more a readability tip, doing it one way or the other makes no difference to code performance

Guadalupe Gagnon

Level 37 , Tampa, United States

16 May 2021, 03:26

and if that doesn't work, try making the mostFrequent method a regular (non-static) method. You won't need to pass it an object for it to work, is it will be a method associated with the object and not the class.

Guadalupe Gagnon

Level 37 , Tampa, United States

16 May 2021, 03:43

I'm sorry, there is another big issue with this code. When a program starts it begins a thread I refer to as the "main" thread. This thread runs, line by line, starting in the main method. In the above code it is responsible for reading fileNames, creating ReadThread objects "t", starting those objects, then putting the fileName and the t.frequent in the map. The last task is where there is a problem. The main thread does not wait around until the ReadThread object it starts each loop cycle finishes its task, it puts the fileName and t.frequent immediately into the map while the ReadThread is working. At this point that will result default value of 0 being added to the map. Your code will not pass the way it is. Take line 25 out and move it to a spot that guarentees that "frequent" will have the correct value (i recommend after the algorithm that finds the most frequent byte). Also, because that would mean that threads are accessing a shared resource (the results map), you will need to ensure that any access is synchronized with an appropriate lock.

Oliver Heintz

Level 18 , Mustang, United States

16 May 2021, 04:42

Oh, man, thanks a lot. That's why it worked before I used threads, and wouldn't work after. I'll mess around with it a bit, but I think I understand what you're saying needs to be fixed. And thanks, too, for all the comments on cleaning up my code. Some of the things you said I knew, but in my rush to type up things I glossed over the details and went too much on just reflexive memory. Usually the red squigglies will remind me to do better, but apparently that's not always the case. In regards to getting the last entry, as I didn't know that HashMaps had indexes, would I use Map.Entry<>lastEntry()? Or, if I changed my comparator to o2.getValue().compareTo(o1.getValue()), I could use Map.Entry<>firstEntry()?

Guadalupe Gagnon

Level 37 , Tampa, United States

16 May 2021, 16:07

You can't access Map items by index, but at line 53 the code converts the Map into a List<Map.Entry> (a list of the map entries) so that it can be sorted, and you can access each item in a List by index. That means, after it is sorted, you can just get the last value (or the first value if sorted in reverse) from the list and it will be your your most frequent byte.