Thread: Cant find stupid bug.

Could you please edit your post so that it's readable: put [code] at the start of a section of code and [/code] at the end.

-----

Think about (and maybe document in the code!) what the public methods of FrequencyAnalysis are supposed to do. That way you can test each one to see that they do what they are supposed to do.

One thing to remember is that in gets input (scans) a stream. A stream is like its real world counterpart - always flowing - so that once you've read the stream with next() you don't get a chance to read that part of the stream again: the stream moves on. Consequently a statement of the intent of each method should include not merely what it returns, but also where it leaves the scanner pointing within the stream.

Alright, I edited it. I did test the methods prior to posting. The countLetters() method is not relevant to this part of the code, but I included it anyway as to prepare for what I have to do when I get this fixed. But that method and the getSent() method both work fine when tested separately. It's the getOccurance() method that doesnt work properly. As to the last part, I think I understand what you are saying, but I dont know by what means to fix that.

double post removed

The countLetters() method is not relevant to this part of the code

It turns out that countLetters() is extremely relevant if you take into account that this method alters where in the input stream your scanner is pointing. System.out.println() can be used to illustrate this explicitly:

import java.io.IOException;
import java.io.StringReader;
import java.util.Scanner;
public class FrequencyTester
{
    public static void main(String [] args) throws IOException
    {
        //Scanner in = new Scanner(new File ("Sub.txt"));
        Scanner in = new Scanner(new StringReader("foo bar"));
        FrequencyAnalysis obj = new FrequencyAnalysis(in);
 
        System.out.println("Before countLetters() hasNext() = " + in.hasNext());       
        obj.countLetters();
        System.out.println("After countLetters() hasNext() = " + in.hasNext());       
 
        for (int i  : obj.getOccurance(obj.getSent()))
        {
            System.out.println(i);
        }
    }
}

This is basically your code but with the scanner adjusted in order to facilitate debugging. As you can see from the output of that code, the call to countLetters() has a profound (side)effect on the scanner. After you have read the entire stream counting the letters hasNext() will return false (and next() will throw an exception). In particular getSent() will *always* return an empty string. It is within that empty string that you are counting characters.

The "fix" is to for the scanner to read the input exactly once. Either put all of the next() calls within the constructor. Or if you want the scanning in a separate method, do that. The advantage of putting the next() calls within the constructor is that you do not need the FrequencyAnalysis class to have a private Scanner in field. And the advantage of not having that field is that the other methods will not be able to try and use that scanner thereby shooting you in the foot.

Bottom line: try doing all the scanning in the constructor. Build up a lowercase string if you like, or just do all the frequency counting as the stream is read.

Wow, that makes a lotttt of sense! Thanks for the help! I attempted what you suggested, and I think I did it right, however I now get an error when trying to make a substring of the string. I have no idea why. If I outprint the String, it is exactly how it is supposed to be. Here is my new code for FrequencyAnalysis:

import java.util.*;
import java.io.*;
public class FrequencyAnalysis
{
  //private Scanner in = new Scanner(new File("Sub.txt"));
  private Scanner in;
  private int lets = 0;
  private int[] occurance = new int[26];
  private String file = "";
  private String[] alph = {"a", "b", "c", "d", "e", "f", "g", "h", "i", "j", "k", "l",
          "m","n", "o", "p", "q", "r", "s", "t", "u", "v", "w", "x", "y", "z"};
 
  public FrequencyAnalysis (Scanner in)
  {
      this.in = in;
 
      for (int i = 0; in.hasNext(); i++)
      file += in.next();
    }
 
    public int countLetters()
    {
        for (int i = 0; this.in.hasNext(); i++)
        {
            String s = this.in.next();
            lets += s.length();
        }
        return lets;
    }
 
    public String getFile()
    {
        return file.replaceAll("[^a-zA-Z0-9]", "").toLowerCase();
    }
 
    public int[] getOccurance(String file)
    {
       for (int i = 0; file.length() != 0; i ++)
       {
           int c = 0;
           while (c < 26)
           {
               if (alph[c].equals(file.substring(0, 1)))
               {
                   occurance[c] += 1;
                   file = file.substring(1);
                   c = 0;
                }
 
                else
                {
                    c++;
                }
            }
 
        }
        return occurance;
    }
}

First of all you still scan the stream in two places. In particular you read it again in countLetters(), and I predict this will not be good:

import java.io.IOException;
import java.io.StringReader;
import java.util.Scanner;
public class FrequencyTester
{
    public static void main(String [] args) throws IOException
    {
        //Scanner in = new Scanner(new File ("Sub.txt"));
        Scanner in = new Scanner(new StringReader("foo bar"));
 
        System.out.println("Before analysis constructor hasNext() = " + in.hasNext());       
        FrequencyAnalysis obj = new FrequencyAnalysis(in);
        System.out.println("Before countLetters() hasNext() = " + in.hasNext());       
        int count = obj.countLetters();
            // :(
        System.out.println("countLetters() returned " + count);
 
            // the following is what I imagine you are saying now
        for (int i  : obj.getOccurance(obj.getFile()))
        {
            System.out.println(i);
        }
    }
}

-----

However I don't wish to distract you from your current problem which lies elsewhere...

substring() will throw an exception when the substring you are trying to find does not exist. In this case the message we see is:

Exception in thread "main" java.lang.StringIndexOutOfBoundsException: String index out of range: 1
	at java.lang.String.substring(Unknown Source)
	at FrequencyAnalysis.getOccurance(FrequencyAnalysis.java:44)
	at FrequencyTester.main(FrequencyTester.java:18

(By the way, it is a *very* good idea to post stack traces like this so everyone knows what we are talking about. And say which lines of your code are being referred to: in this case it is the condition in the if statement of getOccurance())

Now the only way that the index 1 can be out of range is if file is the empty string.

So the first thing we do is check that we did assign something to file:

public FrequencyAnalysis (Scanner in)
{
    this.in = in;
 
    for (int i = 0; in.hasNext(); i++)
    file += in.next();
    System.out.println("analysis constructed! file length = " + file.length());
}

Then we check that getOccurance() is passed a string that has some contents:

public int[] getOccurance(String file)
{
    System.out.println("getOccurance() called! file length = " + file.length());
    for (int i = 0; file.length() != 0; i ++)
   {
        // etc

At this point - if you have been running these variations on your code - it should become apparent that something in getOccurance() is taking the perfectly good nonempty string file and making it empty. That can only be the line:

file = file.substring(1);

because that is the only line that alters the value of file.

(Notice - or put away for future reflection - that we are again seeing a side effect. Here is a method that is supposed to be doing some counting, but in fact it does more than that: it alters the string that file refers to. Whereever you can, have methods do their work without side effects.)

-----

In my opinion your getOccurance() is too complicated.

It is supposed to return an int array of frequencies, right? So why the nested loops? Just have it step down the string with the for loop, use the String method charAt() to get the character c at that position and use it to update alpha[c]. charAt(i) is much better than the substring(0,1) + file=file.substring() combination you are using at the moment.

You might have to check that c is a "good" character, but you don't have to resort to nested loops and, in particular, you won't have that "file=file.substring()" line which is doing no good.

Alright, well i'll start by saying that I tried to write this program once before, when I ran into problems, my instructor advised me to rewrite the entire program. So I did. And I made it much simpler. But Still got the same problem. Thats why I came here. But the point is, in my other program I did try and use charAt(). It didnt work well. Doing that would mean I have to make the String array of letters an array of chars. At least thats what it said last time. But to the "problem" line, that line was on purpose, as it was supposed to take the first character out of the string. I guess it didnt work and took the whole thing? Thats what confuses me. Sorry for being a little bit noobish about not posting the error and stuff. This is my first time seeking help besides google. Haha. But I guess tomorrow I will try out the charAt() method again and see what happens. Thanks for all your help so far!

But to the "problem" line, that line was on purpose, as it was supposed to take the first character out of the string. I guess it didnt work and took the whole thing?

Well, it "works" OK. Computer code always does, so long as it compiles. It's when its working doesn't conform to our intentions that we have a problem...

In this case file=file.substring(1) will be fine until it hits the last character of the string. Then it makes file an empty string and then the next time around the while loop there will be a problem with .equals(file.substring(0,1)).

As I said chatAt() is a more straight forward way to go. But I realise that I misspoke before: it is not quite as simple as seeing that you have character c and then updating alpha[c]. And now I see what you were trying to do with that alph[] String array...

Once you hit a character c in the string file you have to update the entry in the alpha array that corresponds to that character. Perhaps the following may give some insight about how to do that:

public class Foo {
    public static void main(String[] args) throws Exception {
        String test = "foo";
 
        for(int i = 0; i < test.length(); i++) {
            char c = test.charAt(i);
            System.out.println("Found " + c);
 
            System.out.println("    this is character number " + (int)c);
            System.out.println("    and is the " + (c - 'a') + "-th letter of the alphabet");
        }
    }
}

The expression (c - 'a') is a common way of converting between a character and its place in the alphabet which is precisely what you are trying to do. It is the value of alpha[c-'a'] that needs to get updated as you count the frequencies.

Thank you soooooo much! It works perfect now! You have taught me a lot with this! Much love! <3

You're welcome.

Welllllll I ran into an issue :/ It's not counting right. I get like 114 s's when there is only 59 letters, and that doesnt make sense to me. Ill post the code. You'll see that I've also added another part to it that doesnt work perfect: A percentage finder. It is supposed to tell the percentage of the message that the letter takes up.

import java.util.*;
import java.io.*;
public class FrequencyAnalysis
{
  //private Scanner in = new Scanner(new File("Sub.txt"));
  private Scanner in;
  private int lets = 0;
  private int[] occurance = new int[26];
  private String file = "";
  private double [] percent = new double[26];
  private String[] alph = {"a", "b", "c", "d", "e", "f", "g", "h", "i", "j", "k", "l",
          "m","n", "o", "p", "q", "r", "s", "t", "u", "v", "w", "x", "y", "z"};
 
  public FrequencyAnalysis (Scanner in)
  {
      this.in = in;
 
      for (int i = 0; in.hasNext(); i++)
      {
      file += in.next();
    }
 
    lets = file.replaceAll("[^a-zA-Z0-9]", "").toLowerCase().length();
  }
 
  public String[] getAlph()
  {
      return alph;
    }
 
    public int getLets()
    {
        return lets;
    }
 
    public String getFile()
    {
        return file.replaceAll("[^a-zA-Z0-9]", "").toLowerCase();
    }
 
    public int[] getOccurance(String file)
    {
       for (int i = 0; i < lets; i ++)
       {
           char c = getFile().charAt(i);
           occurance[c - 'a']++;
        }
        return occurance;
    }
 
    public double[] getPercent()
    {
        int c = 0;
        for (int i : occurance)
        {
            percent[c] = (i / lets) * 100;
            c++;
        }
        return percent;
    }
}

import java.util.*;
import java.io.*;
public class FrequencyTester
{
    public static void main(String [] args) throws IOException
    {
        Scanner in = new Scanner(new File ("Sub.txt"));
        FrequencyAnalysis obj = new FrequencyAnalysis(in);
 
       System.out.println("Letter   Times occured   Percentage of file");
 
        for (int i = 0; i < 26; i++)
        {
            System.out.print("   " + obj.getAlph()[i]);
         System.out.printf("%11s", obj.getOccurance(obj.getFile())[i]);
         System.out.printf("%19s%n", obj.getPercent()[i]);
    }
 
}
}

public int[] getOccurance(String file)
{
   for (int i = 0; i < lets; i ++)
   {
       char c = getFile().charAt(i);
       occurance[c - 'a']++;
    }
    return occurance;
}

Every time you call getOccurance() the elements of the array like occurance[c-'a'] get incremented which leads to inflated values. This is similar to the problem with the scanning. And the solution is the same: update the elements of the occurance array just once, probably in the constructor as you scan the input.

Thats confusing... I only call getOccurance() once, and the elements should only be getting incremented when they match the char that c is set as. Thats what [c - 'a'] does, correct?

for (int i = 0; i < 26; i++)
{
     System.out.print("   " + obj.getAlph()[i]);
     System.out.printf("%11s", obj.getOccurance(obj.getFile())[i]);  //<-------
     System.out.printf("%19s%n", obj.getPercent()[i]);
}

No, you call getOccurance() twenty six times within the loop.

*Facepalm* That was a dumb mistake. Thanks! I didnt realize what was going on there!

I chose to go the route of setting up that method as setOccurance() and making it void and using a getOccurance() method to return it to print. That works fine, at least it seems to. The problem comes when trying to implement my setPercent() method. When I outprint it, there are letters that have been counted in occurance one or more times, and it shows 0% for the percentage. Also when tested on a bigger file, it counted over 600 z's, which is realllly unlikely, but other than that, it still said 0%! I am confounded here.

import java.util.*;
import java.io.*;
public class FrequencyAnalysis
{
  //private Scanner in = new Scanner(new File("Sub.txt"));
  private Scanner in;
  private int lets = 0;
  private int[] occurance = new int[26];
  private String file = "";
  private double [] percent = new double[26];
  private String[] alph = {"a", "b", "c", "d", "e", "f", "g", "h", "i", "j", "k", "l",
          "m","n", "o", "p", "q", "r", "s", "t", "u", "v", "w", "x", "y", "z"};
 
  public FrequencyAnalysis (Scanner in)
  {
      this.in = in;
 
      for (int i = 0; in.hasNext(); i++)
      {
      file += in.next();
    }
 
    lets = file.replaceAll("[^a-zA-Z0-9]", "").toLowerCase().length();
  }
 
  public String[] getAlph()
  {
      return alph;
    }
 
    public int getLets()
    {
        return lets;
    }
 
    public String getFile()
    {
        return file.replaceAll("[^a-zA-Z0-9]", "").toLowerCase();
    }
 
    public void setOccurance(String file)
    {
       for (int i = 0; i < lets; i ++)
       {
           char c = getFile().charAt(i);
           occurance[c - 'a']++;
        }
    }
 
    public int[] getOccurance()
    {
        return occurance;
    }
 
    public void setPercent()
    {
        for (int c = 0; c < 26; c++)
        {
            percent[c] = ((double)occurance[c] / (double)lets) * (double)100;
            c++;
        }
    }
 
    public double[] getPercent()
    {
        return percent;
    }
}

 
import java.text.*;
import java.util.*;
import java.io.*;
public class FrequencyTester
{
    public static void main(String [] args) throws IOException
    {
        Scanner in = new Scanner(new File ("Sub.txt"));
        FrequencyAnalysis obj = new FrequencyAnalysis(in);
 
       System.out.println("Letter   Times occured   Percentage of file");
        obj.setOccurance(obj.getFile());
        obj.setPercent();
        DecimalFormat df = new DecimalFormat("#.###");
        for (int i = 0; i < 26; i++)
        {
            System.out.print("   " + obj.getAlph()[i]);
         System.out.printf("%11s", obj.getOccurance()[i]);
         System.out.printf("%20s%n", df.format(obj.getPercent()[i]) + "%");
    }
 
    PrintWriter out = new PrintWriter(new File("subsetfreq.txt"));
 
    out.println("Letter   Times occured   Percentage of file");
    for (int i = 0; i < 26; i++)
        {
            out.print("   " + obj.getAlph()[i]);
         out.printf("%11s", obj.getOccurance()[i]);
         out.printf("%20s%n", df.format(obj.getPercent()[i]) + "%");
    }
}
}

OK - so you have to debug.

First - not actually needed here, but a good idea anyway - use the trick I suggested earlier of replacing the scanner with one that merely reads a short string. The point is that you want some reasonably small amount of data where you know the results that should be obtained. Also it makes it easier or anyone here to actually run your code and see what you see (without having to muck about with a data file.)

public static void main(String [] args) throws IOException
{
    //Scanner in = new Scanner(new File ("Sub.txt"));
    Scanner in = new Scanner(new StringReader("aaa asd abc"));
    FrequencyAnalysis obj = new FrequencyAnalysis(in); 
 
    // etc

Next, it appears the percentages are not being calculated correctly. So include a System.out.println() to say what is being calculated:

public void setPercent()
{
    for (int c = 0; c < 26; c++)
    {
        percent[c] = ((double)occurance[c] / (double)lets) * (double)100;
        System.out.println("Set percent[" + c + "] to " + percent[c]);  // <------ add this
        c++;
    }
}

Aha! I see, so by the looks of that its skipping every other one with calculating percent, because of:

public void setPercent()
{
    for (int c = 0; c < 26; c++)
    {
        percent[c] = ((double)occurance[c] / (double)lets) * (double)100;
        System.out.println("Set percent[" + c + "] to " + percent[c]);  // <------ add this
 
        c++;  <------ this bad boy right here. it screws with the c++ in the for loop parameters.
    }
}

Yes, exactly.

Hmmm.. after changing that it seems to work alright with the small file, however with the bigger one I get an error:

java.lang.ArrayIndexOutOfBoundsException: -48
at FrequencyAnalysis.setOccurance(FrequencyAnalysis.j ava:53)
at FrequencyTester.main(FrequencyTester.java:20)

And I get that its out of the limits of the array, but I dont understand how.

Also, could you tell me why it is not printing to a file like it should? It makes the file, but its empty.

Using this tester.

import java.text.*;
import java.util.*;
import java.io.*;
public class FrequencyTester
{
    public static void main(String [] args) throws IOException
    {
        Scanner in = new Scanner(new File ("Sub.txt"));
         //Scanner in = new Scanner(new StringReader("aaa asd abc"));
        FrequencyAnalysis obj = new FrequencyAnalysis(in);
 
       System.out.println("Letter   Times occured   Percentage of file");
        obj.setOccurance(obj.getFile());
        obj.setPercent();
        DecimalFormat df = new DecimalFormat("#.##");
        for (int i = 0; i < 26; i++)
        {
            System.out.print("   " + obj.getAlph()[i]);
         System.out.printf("%11s", obj.getOccurance()[i]);
         System.out.printf("%20s%n", df.format(obj.getPercent()[i]) + "%");
    }
 
    //PrintWriter out = new PrintWriter (new File("plaintextfreq.txt"));
 
    PrintWriter out = new PrintWriter (new File("subfreq.txt"));
 
    out.println("Letter   Times occured   Percentage of file");
 
    for (int i = 0; i < 26; i++)
        {
            out.print("   " + obj.getAlph()[i]);
         out.printf("%11s", obj.getOccurance()[i]);
         out.printf("%20s%n", df.format(obj.getPercent()[i]) + "%");
    }
}
}

I get an error:

java.lang.ArrayIndexOutOfBoundsException: -48
at FrequencyAnalysis.setOccurance(FrequencyAnalysis.j ava:53)
at FrequencyTester.main(FrequencyTester.java:20)

And I get that its out of the limits of the array, but I dont understand how.

Which is line 53? Remember that you calculate the array position with something like c-'a' which could end up with a bad value like -48 depending on the value of c. To debug use System.out.println() somewhere before line 53 to see what this character is. Some characters are not easy to read when they are printed to the console so that you have to resport to something along the lines of

System.out.println("c is -->" + c + "<--"); // lets us tabs, new lines etc
System.out.println("c is " + (int)c); // prints the numeric value as a decimal int

-----

why it is not printing to a file

What does "not printing" mean? Specifically, is there a runtime error? is the file not created? is the file created but doesn't have the content you expect? something else?

Remember you should close streams after you are finished using them.

Great help! Turns out it ended up being a number! But that doesnt make sense to me because this should have removed numbers:

    public String getFile()
    {
        return file.replaceAll("[^a-zA-Z0-9]", "").toLowerCase();
    }

Yes. After a little arithmetic and

//Scanner in = new Scanner(new File ("Sub.txt"));
Scanner in = new Scanner(new StringReader("aaa1 1asd 1 abc 1"));

I see the same thing.

But doesn't that regular expression mean remove everything that is not a letter or digit? So digits like '1' get left in. Are you trying to count them? because that would make things a little awkward (their values are nowhere near those of letters, so the c-'a' idiom won't be as effective).