views:

359

answers:

7

The following piece of code should read each line of the file and operate on it. However, it only reads the first line. Without the for loop it reads the whole file. I honestly have no idea why it's not reading the whole thing.

StreamReader sr = new StreamReader(gridPath);

string line;
char[] lineCh;
char current;
int x, y;
bool north, east, south, west;

x = y = 0;

while ((line = sr.ReadLine()) != null)
{
    lineCh = line.ToCharArray();
    for (int i = 0; i < lineCh.Length; i++)
    {
        current = lineCh[i];
        north = CheckInput(current);
        current = lineCh[++i];
        east = CheckInput(current);
        current = lineCh[++i];
        south = CheckInput(current);
        current = lineCh[++i];
        west = CheckInput(current);
        i++; // Hop over space
        grid[x, y] = new GridSquare(north, east, south, west);
        x++; // Start next column
    }
    Console.WriteLine(line);
    y++;
}

Without the for loop the following works and prints the whole file:

StreamReader sr = new StreamReader(gridPath);

string line;
char[] lineCh;
char current;
int x, y;
bool north, east, south, west;

x = y = 0;

while ((line = sr.ReadLine()) != null)
{
    lineCh = line.ToCharArray();

    Console.WriteLine(line);
    y++;
}

sr.Close();     

CheckInput is as follows:

private bool CheckInput(char c)
{
    switch (c)
    {
        case 'y':
            return true;
        case 'n':
            return false;
        default:
            return true;
    }
}

A sample input file:

nyyn nyyy nyyy nyyy nyyy nnyy
yyyn yyyy yyyy yyyy yyyy ynny
yyyn yyyy yyyy yyyy ynyy nnnn
yyyn yyyy yyyy yyyy ynyy nnnn
yyyn yyyy yyyy yyyy yyyy nnyy
yynn yyny yyny yyny yyny ynny
A: 

I think your problem may be...

for (int i = 0; i < lineCh.Length; i++)

Combined with the many ++i statements.

Here's the code with a load of comments... assumes each line is "1234".

        StreamReader sr = new StreamReader(gridPath);

        string line;
        char[] lineCh;
        char current;
        int x, y;
        bool north, east, south, west;

        x = y = 0;

        while ((line = sr.ReadLine()) != null)
        // line is "yyyy"
        {
            lineCh = line.ToCharArray();
            // lineCh.Length is 4
            for (int i = 0; i < lineCh.Length; i++)
            {
                current = lineCh[i]; // i is zero
                north = CheckInput(current);
                current = lineCh[++i]; // i is 1
                east = CheckInput(current);
                current = lineCh[++i]; // i is 2
                south = CheckInput(current);
                current = lineCh[++i];  // i is 3
                west = CheckInput(current);
                i++; // Hop over space // i is 4
                grid[x, y] = new GridSquare(north, east, south, west);
                // (true,true,true,true)
                // So essentially the loop ends if there are four,
                // or goes round again for multiples of 4 - of course,
                // it will error if there is ever 3, or 5 or any other non multiple of 4

                x++; // Start next column
            }
Sohnee
why is this a problem? ++i is not the same as the end of the char array. Presumably there are more than 1 gridsquares in 1 line.
PoweRoy
I have commented up the code sample to help clarify...
Sohnee
It only needs to work with my "official" inputs, a sample of which I have now given
tsv
Will i not get incremented again at the start of the loop? The next loop would start with i==5, where it should be 4.
Jens
yes, but that should be "yyyy n" the "n" is pos 5
PoweRoy
This seems like a massive anti-pattern to me. The data should be serializes so it is readable. The code is highly dependent on the data format, and a single character could cause an exception.
Sohnee
+6  A: 

Are you getting an exception in the for loop? You are incrementing i, maybe at some point you are trying to index incorrectly lineCh.

EDIT: another candidate for bad indexing is the grid array. I see no initialization code, and the values of x and y are determined after reading the file. How do you initialize it?

Paolo Tedesco
+1 because it's the only possibility that makes sense.
Mehrdad Afshari
No exception from the loop. It reads one whole line fine; surely it should reset itself and read the others fine too?
tsv
+1 and likely a lesson learned on what exceptions are
kenny
@tsv: How did you check and reached the conclusion that the loop doesn't throw an exception and it's not somewhere else that catches it?
Mehrdad Afshari
@Mehrdad if you count through the loop with the example input you should see there is no reason for an exception to be thrown.
tsv
@tsv: So far you've looked at your source and you couldn't see a reason it doesn't work. You are simply *speculating* that it doesn't throw an exception, just like you had speculated that the code runs correctly (and it didn't). This is not how debugging works. You should run the code, have the debugger break as soon as an exception is thrown (Ctrl+Alt+E in VS, check the Thrown box near CLR exceptions) and make sure if no exception is thrown. If it wasn't throwing, then you should step into the code and carefully check to see what's wrong with it.
Mehrdad Afshari
+1  A: 

your code throws exception because you can get out array bounds at any of those lines:

current = lineCh[++i];
Andrey
+1. Precisely the problem!
Fadrian Sudaman
+3  A: 

You are modifying a loop control variable inside the body of your loop, this is something you should avoid as it lead to unexpected execution of your loop.

Please show a sample of the line that you are trying to process and I maybe able to suggest a better implementation of your for loop.

Do you need to process the entire line at once or do you need to break it into chunks of 4 characters, process these 4, then move onto the next line?

You could try changing how you process the line:

        while ((line = sr.ReadLine()) != null)
        {
            string[] segments = line.Split(' ');

            foreach(string segment in segments)
            {
                char[] arr = segment.ToCharArray();
                north = CheckInput(arr[0]);
                east = CheckInput(arr[1]);
                west = CheckInput(arr[2]);
                south = CheckInput(arr[3]);
                grid[x, y] = new GridSquare(north, east, south, west);
            }


            Console.WriteLine(line);
            y++;
        }

Here I split the line based on the spaces, then I can operate on the individual segment by splitting in into a character array and accessing the specific characters.

This code also makes the assumption that there will always be 4 characters to each segment, will this always be the case? You should also add validation to ensure that the line is what you expect.

Alan
Have now added input example.
tsv
Good thinking outside the box. Even you can leave the ToCharArray
PoweRoy
Thanks, provided me with much cleaner code which allowed me to realise the actual error!
tsv
A: 

It is dangerous to increment the looping variable inside the loop itself. I would recommend to create a custom type for your north, east, etc. variables and then consume each line to the end. Or maybe even better return the next GridSquare object.

This could be done with a method returning an iterator for GridSquares:

StreamReader sr = new StreamReader("input.txt");

string line;
char[] lineCh;
char current;
int x, y;
bool north, east, south, west;

x = y = 0;

while ((line = sr.ReadLine()) != null)
{
    foreach (var gs in GetGridSquares(line))
    {
        // grid[x, y] = gs;
    }

     Console.WriteLine(line);
     y++;
 }

GetGridSquares is:

 private IEnumerable<GridSquare> GetGridSquares(string line)
    {
        var splittedLine = line.Split(' ');
        foreach (var gsStr in splittedLine)
        {
            if (gsStr.Length != 4)
            {
                continue;
            }

            yield return new GridSquare(gsStr[0], gsStr[1], gsStr[2], gsStr[3]);
        }
    }
schoetbi
A: 
StreamReader sr = new StreamReader(gridPath);

var line;       
var y = 0;  

while ((line = sr.ReadLine()) != null)
{
    for(var i =0; i<line.length;i+=2)
    {
        grid[i,y]=new GridSquare(GetBits(line[i],i));
        grid[i+1,y]=new GridSquare(GetBits(line[i],i+1));


    }
    ++y;

}

bool [] GetBits(char bBytes, int n)
{
    var returned = new bool[4];
    bBytes = bBytes << ((n%2)*4);
    for(var i =0; i < 4; ++i)
        returned[i]=(bBytes & (1<<i ) > 0;

}
imbz
A: 

The actual answer I found after cleaning the code was that x is not being set back to zero ever; we never move to the next row of the grid[,]. I realise this was hard to work out from my examples, my apologies there.

tsv