Access ArrayList Getter from inner class

[Josh Herron]

Good evening everyone.   I'm very new to Java and I've been trying to get through all of the content on Pluralsite to help me learn this.   That being said, I don't really learn well that way so I thought I would try to create a small application to parse a text file that my friend uses for his IPTV application.

I have managed to create a simple GUI with a single button that opens a txt file.  And then that file is read and some unique ID's are gathered and if they don't already exist in my ArrayList I'm adding them.  If I print the ArrayList value from the file analyzeFile method I can see all of the values as I would expect.   However after the analysis is done control is passed back to the GUI and if I call the getListGroupIDs method I am getting back an empty ArrayList.   I assume this is because my Action Listener class is an inner class and the ArrayList is not available to it.  I have tried everything I can think of (changing the ArrayList to Public, Private, blank) and nothing works. 

Here is where I declare my ArrayList and the setter/getter I created.

ArrayList<String> listGroupIDs = new ArrayList<>();
	
	void setGroupIDs (String groupIDs) { 
		 this.listGroupIDs.add(groupIDs);
	}
	
	List<String> getListGroupIDs(){
		 return listGroupIDs; 
	}

Here is the code that analyzes the text file.   I know it's not very good, but it does work.   (I would be thrilled if anyone can point out some things that I could try to do better)

	public void analyzeFile (File file) throws IOException{
		
		String match = "group-title=";
		String match2 = ",";
		String text = null;
		FileReader in = new FileReader(file);
		BufferedReader br = new BufferedReader(in);
		String str = "empty";
		FileCleaner value = new FileCleaner();
		

    while ((text=br.readLine()) != null) {
    	if (text.toLowerCase().indexOf(match.toLowerCase()) > 0){
    		int loc = text.indexOf(match);
    		text.toLowerCase().indexOf(match2.toLowerCase());
    		int loc2 = text.indexOf(match2);
    		loc = loc + 13;
    		loc2 = loc2 - 1;
    		str = text.substring(loc, loc2);

    		
    		if(value.getListGroupIDs().contains(str)) {
    			br.readLine();
    			System.out.println("list contained string " + str);
    		} else {
    			value.setGroupIDs(str);
    			System.out.println("ArrayList add: " + value.getListGroupIDs());
    		}
    	} else {
    				System.out.println("line skipped");		
	}
}	
    System.out.println("end of file");
    in.close();
    return;	
}  // end analyzeFile
	

And last but not least... after the analyzeFile method is complete control is passed back to the class that called the method  (the JFileChooser action) and when I run getListGroupIDs from here I get a blank ArrayList.

public class OpenFileListener implements ActionListener  {
		public void  actionPerformed(ActionEvent ev) {
			
		FileCleaner run = new FileCleaner();
		
		 JFileChooser fc = new JFileChooser();
		 fc.showOpenDialog(frame);
		 try {
			analyzeFile(fc.getSelectedFile());
			System.out.println("File complete");
			System.out.println("The value of my arraylist is " + run.getListGroupIDs());
		} catch (IOException e) {
			// TODO Auto-generated catch block
			e.printStackTrace();
		}
	}		 
	
}   // end OpenFile Listener

I apologize for the super long post, but I'm just driving myself crazy with the failure here.   I don't personally know anyone that I could even bounce ideas off of, so I'm hoping someone here can shed some light on what I'm doing wrong.   I appreciate any feedback I can get! 

[Campbell Ritchie]

Welcome to the Ranch

Did they really advise you to use a GUI for reading a file? As a general rule, get your application working without a GUI (i.e. reading from the file) and the add a GUI later.
What's IPTV.
Why are you using a List to add things to if it doesn't already contain them? My concept of a List is that it is supposed to contain duplicates, so why not use another datatype, possibly a kind of Set, which doesn't support duplicates already.

I am not sure why you are getting an empty List returned. Let's have a look at what you have posted, however.
Which class is the List declared in? I presume that is an instance field (which shou‍ld be private). You don't have a setXXX method, because it doesn't set the List, so I think you shou‍ld change its name. It is adding something not setting it. I think a List<GroupID> would be better than a List<String> but creating a GroupID class is probably not your first priority just at the moment. Similarly your getXXX method is a bit hazardous. I know of four ways to return a reference to a List, which I have written before, but can't seem to find:-

1: Return the List as is, which is the obvious way to do it but anybody getting a reference to that List can change it, so that is probably a bad idea.

2: Return a read‑only copy: return Collections.unmodifiableList(myList); That is a read‑only copy; subsequent changes to the original List are reflected in the unmodifiable copy.

3: Return a new List: return new ArrayList(myList); That is a competely independent List; if the recipient changes it, your class will never know anything about it.

4: Return an unmodifiable copy: return Collections.unmodifiableList(new ArrayList(myList)); That List cannot be changed, so it is an immutable snapshot of the state of your object at the time the getXXX method was called. The recipient can of course copy that List into a new List.

Remember that the state of any mutable objects in an immutable List can still be changed; that is not a problem with things immutable like Strings. You will have to decide which option fits your requirements best. No 2 might be best, but you have to decide that for yourself.

You didn't say in your action listener how you are using the run local variable. You are creating a file cleaner object (run is probably not a good name for that) and I can't see any way you are adding anything to it. You have a call to get the List, but I I can't seem to see where you are adding anything to the List.Then you have another file cleaner object in the analyse file method, and I can't seem to see anything being added to that.

Beware: you now have two file cleaner objects which operate completely independently of each other.

Watch this space.

[Campbell Ritchie]

We get much longer posts, and at least you have explained the problem and laid out your code clearly; that will get some recognition later

You didn't say what is in the file, but you did say it is a text file. Consider whether you want to read the file line by line or token by token. The Scanner documentation has a simple example of token by token reading from a file full of longs.
Whichever method of reading you use, you must close the object reading the file, otherwise there is a risk of a resource leak; under certain circumstances the file may remain locked and inaccessible to new applications. I think the best way to close the reading is try-with-resources. That will also mean you will handle any exceptions here, rather than simply declaring them with a throws clause.
But back to reading.
Your method doesn't analyse the file; it reads the file, tests for matches, adds the tokens to a List (or doesn't add, possibly) and a few more things. That ain't good practice. Use one method for reading. Use one method to do the matching. Use one method to test for adding to the file. Or something like that. I am not sure of the best way to do it, but that method is too big.
You didn't say what you are parsing; I am going to have to write my own parser soon, so I shall probably have a class like

public interface TypedToken
{
   Type getType();
   String getText();
}
// *********
public class Token implements TypedToken
{
   private final String text;
   private final Type type;
   // constructor, getXXX methods, toString, hashCode, and equals
}
// *********
public enum Type
{
   KEYWORD, NUMBER_LITERAL, TEXT_LITERAL,
      IDENTIFIER, BRACKET, OPERATOR;
}
/*
 * It is possible to enhance that with enums for different
 * operators along with the types they operate on, and their
 * precedences. + operates on numbers and ∩ operates on sets.
 */

I am now getting ideas for my own work. As I go through the file, I can make a Token instance for each token I encounter. That might be worth your while considering.

[Campbell Ritchie]

Please consider whether you shouldn't make that inner class and the analyse file method private.

[Josh Herron]

Campbell Ritchie wrote:Welcome to the Ranch

Did they really advise you to use a GUI for reading a file? As a general rule, get your application working without a GUI (i.e. reading from the file) and the add a GUI later.
What's IPTV.
Why are you using a List to add things to if it doesn't already contain them? My concept of a List is that it is supposed to contain duplicates, so why not use another datatype, possibly a kind of Set, which doesn't support duplicates already.

I am not sure why you are getting an empty List returned. Let's have a look at what you have posted, however.
Which class is the List declared in? I presume that is an instance field (which shou‍ld be private). You don't have a setXXX method, because it doesn't set the List, so I think you shou‍ld change its name. It is adding something not setting it. I think a List<GroupID> would be better than a List<String> but creating a GroupID class is probably not your first priority just at the moment. Similarly your getXXX method is a bit hazardous. I know of four ways to return a reference to a List, which I have written before, but can't seem to find:-

1: Return the List as is, which is the obvious way to do it but anybody getting a reference to that List can change it, so that is probably a bad idea.

2: Return a read‑only copy: return Collections.unmodifiableList(myList); That is a read‑only copy; subsequent changes to the original List are reflected in the unmodifiable copy.

3: Return a new List: return new ArrayList(myList); That is a competely independent List; if the recipient changes it, your class will never know anything about it.

4: Return an unmodifiable copy: return Collections.unmodifiableList(new ArrayList(myList)); That List cannot be changed, so it is an immutable snapshot of the state of your object at the time the getXXX method was called. The recipient can of course copy that List into a new List.

Remember that the state of any mutable objects in an immutable List can still be changed; that is not a problem with things immutable like Strings. You will have to decide which option fits your requirements best. No 2 might be best, but you have to decide that for yourself.

You didn't say in your action listener how you are using the run local variable. You are creating a file cleaner object (run is probably not a good name for that) and I can't see any way you are adding anything to it. You have a call to get the List, but I I can't seem to see where you are adding anything to the List.Then you have another file cleaner object in the analyse file method, and I can't seem to see anything being added to that.

Beware: you now have two file cleaner objects which operate completely independently of each other.

Watch this space.

Nobody suggested that I use a GUI, I was just trying to make a small application that he can run when a new file is available.   IPTV is an application for watching television from overseas, they produce an .M3U file (that opens as .txt) and that file contains thousands of rows of entries for channels that are grouped by region/language.   I already created an excel file with VBA that opens the file and then cleans it removing all of the languages he doesn't want, but I thought I might go a step further here and put it all in a GUI that allows the user to open a file, scan the file for all of the possible group IDs and then return those to corresponding checkboxes that allow the user to select which ones they want their output file to contain.  

That's why I was adding the unique values to an ArrayList, my very limited understanding of Java told me that an ArrayList can expand without knowing the number of elements that it will contain.    I'll be completely honest, I don't really know about sets yet.   I'm only about 4 weeks into learning Java and the training I've been doing has  just briefly mentioned them, but hasn't done a good job of cementing why/when to use them.

You mention that I need to close my reader, I thought I did that with my in.close();   at the end of my analyzeFile method.  Is that not correct?

The ArrayList is declared in my FileCleaner class which really just holds my instance variables and that ArrayList.   I have this as my initial class in the project and then the Main method just calls my openGUI() method which is what creates the JFrame/JPanel with my single button for opening the file.  Which is where we start all of the fun.   The list that is created in analyzeFile will never change at any point once those values have been identified and set.  (unless the user opts to open a new file)  So I just need that to be a static value that will be used to created checkboxes in the GUI.  

I didn't think the FileCleaner objects were good practice, but I couldn't figure out how to call my methods without doing this, should I have declared my new FileCleaner object at the class level and just used that object going forward?

[Josh Herron]

. . .
I figured different methods for each function is best practice... I just don't fully grasp how to open the file and read each line and then pass that line value to the next method to see if it contains the value I'm looking for; and if so pass the line value again to the next method to determine if the value found already exists in the ArrayList.
. . .

[Josh Herron]

Campbell Ritchie wrote:Please consider whether you shouldn't make that inner class and the analyse file method private.

It was originally private but when my ArrayList starting coming back blank in the OpenFileListener class I started changing them to see if different values would give a different result.

[Campbell Ritchie]

That sounds like guessing. You would be better off following the flow of control with a pencil and finding where you are getting access to that List. Make the fields private, and see what happens. As I said, I didn't notice anywhere that you are adding things to that List. You appear to be reading line by line and appear not to read token by token and you probably don't need OPERATOR/KEYWORD etc.
I missed the close() call. Sorry. Close the buffered reader in preference to the file reader.

I would suggest something like this. Scanners are suitable for text files only. Line 2 ensures the Scanner is closed whether or not an Exception occurs.

private void analyseGroupIdFile()
{
   try
   (Scanner fileScan = new Scanner(Paths.get("myGroupIdFile.txt")))
   {
      while (fileScan.hasNextLine())
      {
         processLine(fileScan.nextLine());
      }
   }
   catch (IOException exc)
   {
      // handle exception
   }
}
// ... What follows is dreadfully oversimplified.
private void processLine(String line)
{
   if (line.matches(myRegex))
   {
      myCollection.add(new GroupID(line));
   }
}

If you use a Set, remember there are different sorts, depending on which order you want to run the Iterator. Sets enlarge to accommodate all elements, too. Note that the add method returns false if you fail to add the element, so that allows you to verify whether you have a duplicate.// ... You can use the ?: operator to shorten lines 21-28 private void processLine(String line) {    if (line.matches(myRegex))    {       if (myCollection.add(new GroupID(line)))       {           System.out.println("added");       }       else       {           System.out.println("duplicate");       }    } }You can use a technique similar to what I showed earlier to return an unmodifiable Set.
I think the only thing I would query about the FileCleaner class is its name. Does it really clean a file? Maybe it shou‍ld have a different name. Maybe not. I am not sure. Otherwise I think it is probably all right.

Please don't quote the whole of previous posts; I shall delete the excess posts.

[Josh Herron]

I'm not sure why my second response wasn't there.   The quote shows up but my response was dropped.  (probably user error on my part)

I will review the scanner documentation it may very well work as well as or better than the buffered reader.  

I know that the single huge method isn't the best way to move forward, but as you can tell I'm a little uncertain how values are passed between methods.  (hence my creating multiple FileCleaner objects)   Can you possibly give me a real brief example of how I might accomplish this?   Do I need to create a new object at the class level and then use that going forward to call each method?

[Josh Herron]

The FileCleaner does not currently actually clean the file, but eventually it will.   The plan is to index through each line and get all valid GroupIDs, allow the user to select the ones they want and then write a new file containing only lines with those values as a new cleaned file.

So I believe in the end following your suggestions I will have my FileCleaner class  and then the following methods.  5-7 are totally imagined, with no work being done in that area yet.

1.  OpenFile
2.  Scan file to find lines containing GroupID
3.  Add GroupID to List or Set
4.  Open GUI
5.  Use List/Set values to create checkboxes for users to select
6.  Use selected checkbox values to rescan txt file
7.  write lines meeting criteria to new text file


Yes, when I changed the access modifiers to Public/private it was totally guessing.   I was beyond frustrated at that point and really should have just stepped away, I only get about 1-2 hours a day to try to learn this and I was irritated that I hadn't made any progress in the last few days.

[Campbell Ritchie]

For line by line reading a buffered reader will probably run faster than a Scanner. You can do it for about the same complexity of code:-

try
(BufferedReader reader =
         Files.newBufferedReader(Paths.get("myGroupIDFile.txt")))
{
   String line;
   while ((line = reader.readLine()) != null)
   {
      processLine(line);
   }
}
catch (...)
{
   // ...
}

You can also get a Stream<String> out of that Path with a different method of the Files class. Note you will probably have to import the NIO package.

[Campbell Ritchie]

Josh Herron wrote:. . . I will have my FileCleaner class  and then the following methods. . . .

That looks good. Maybe some things will eventually appear in other classes.

. . . I was beyond frustrated . . .

Yes, it is annoying, isn't it. We have all experienced such frustration.

[Josh Herron]

Campbell Ritchie wrote:For line by line reading a buffered reader will probably run faster than a Scanner. You can do it for about the same complexity of code:-[code=java]try
(BufferedReader reader =
         Files.newBufferedReader(Paths.get("myGroupIDFile.txt")))
{

Can you provide some additional detail about how to use the Files.newBufferedReader(Paths.get.... functionality in the quote above?   I'm able to open the file using the JChooser but I can't seem to get the file to open when I try this method.  I'm not sure where I am declaring the path to the file, do I set a method variable with the full path to the myGroupIDFile.txt?  

Also, thank you so much for providing guidance without making me feel stupid, I have looked at other forums and many of the users just cut down people asking questions instead of helping them look for ways to improve.

[Campbell Ritchie]

I had forgotten you were using a file chooser. That returns a File object from its getSelectedFile() method. You may find the old way of creating a buffered reader easier; you appear to know it:-

try
(BufferedReader fileReader = new BufferedReader(
                             new FileReader(chooser.getSelectedFile())))
// etc

Have a look through the Java™ Tutorials for more details. You might find the sections about what is a Path? reading files, and legacy code the most helpful. I think the above is more elegant than writing

try
(BufferedReader fileReader = Files.newBufferedReader(
                             Paths.get(chooser.getSelectedFile().getName())))
// etc

. . . or no, maybe the latter is OK after all.

[Campbell Ritchie]

Josh Herron wrote:. . . I'm not sure where I am declaring the path to the file, do I set a method variable with the full path to the myGroupIDFile.txt?

Passing a Path or a File or a String describing the path to the file as a method argument would be a good way to do it. You would write something like
analyseFile(myChooser.getSelectedFile());
or
analyseFiles(myChooser.getSelectedFile().getName());
somewhere, maybe in the action listener's method. Please test the File#getName() method to confirm it gives you the correct output.

. . . thank you so much for providing guidance without making me feel stupid, I have looked at other forums and many of the users just cut down people asking questions instead of helping them look for ways to improve.

That's a pleasure What a nice thing to say about us: thank you.

I have tried out getName and I think you would be better off with this method or similar.

[Campbell Ritchie]

Josh Herron wrote:I'm not sure why my second response wasn't there. . . .

You managed to get it inside the code tags, so I thought it wasn't there at all. I only found it a minute or two ago when I noticed how long the lines in the code tags were. I shall do something to restore it.

[Campbell Ritchie]

Josh Herron wrote:. . .
I figured different methods for each function is best practice... I just don't fully grasp how to open the file and read each line and then pass that line value to the next method to see if it contains the value I'm looking for; and if so pass the line value again to the next method to determine if the value found already exists in the ArrayList.
. . .

That is, as far as I can tell, your second response restored, minus my quoted text.

I hope I have answered those points already; if not don't hesitate to ask.

[Josh Herron]

Just following up...your guidance hit the mark.   I made great progress today.   Tomorrow I will be trying to monitor the JCheckBoxes in my GUI and add the selected labels to a list for use in the actual cleaning step.

Hopefully I won't have to come back with my tail between my legs asking for help again. 

[Campbell Ritchie]

No, you'll be back helping somebody else. Well done