Is there any obvious reason why my loop is not working as it should be?

Question

I have a grid whose (x,y) co-ordinates correspond to a SQL Server. There is not a row for every co-ordinate; only the ones that are not blank. There is a column ("fill") that stores these colour code integers.

When the application loads I have a sequence below that loops through every (X,Y) co-ordinate within the height/width of the grid. Within this loop, it asks whether the (X,Y) in question match those in the row and, if this returns TRUE, updates the Fill integer from that saved in the row. If this returns FALSE, I increase the row by one and look again, and again, until it has looked through every row saved in the table (named SQL.RecordCount). Once this has been done, I increase X (moving right, to the next cell) and do this process again; once I get to the end of the row (where X = MapWidth) I go to the start of the next row and go again. This repeats until Y = MapHeight, as shown in the code below.

To me this sequence makes sense, so there must be something I do not know is missing. Even if you do not have the exact solution, anything to move forward this gridlock would be very much appreciated.

Edits based on comments are in double asterix:

Public Sub LoadMap()

        Dim Fill As Integer = 0
        Dim Row As Integer = 0
        Dim X As Integer = 0
        Dim Y As Integer = 0
              

        SQL.ExecQuery("SELECT * FROM Mapper_Table")

        Do While Y <= MapHeight
            'Ultimate exit statement: stop loading cells when Y becomes greater than the MapHeight...
            Do While X <= MapWidth
                'Row Exit Statement: When X reaches 100, its time to start on the next row...
                Do While Row < SQL.RecordCount '12
                    'I only want to search as many rows as there are saved locations, otherwise the cell must be empty and therefore blank...
                    If X = SQL.DBDT.Rows(Row).Item("X") And Y = SQL.DBDT.Rows(Row).Item("Y") Then
                        'If the current (X,Y) is a match to the database row, then we take the stored colour...
                        Fill = SQL.DBDT.Rows(Row).Item("Fill")
                    End If
                    'If Fill is taken, we can colour the square; otherwise, we should look in the next row...
                     If Fill <> 0 Then
                    Map(X, Y, 0) = Fill
                    End If
          **  Row = Row + 1 **
                    Loop
                'Once we have looked in all the rows for out cell, it is either coloured in or not; we can move onto the next cell in the row; a.k.a...
                X = X + 1
          **  Fill = 0  **
            Loop
            'Once we have looked for all the cells in our row, we can move onto the next one; a.k.a...
            X = 0
            Y = Y + 1
        Loop
        'Once we get to the end of the map, it should be loaded!

    End Sub

Dai · Accepted Answer

Here's how I would do it, albiet in C# (converting this to VB.NET should be straightforward - I just really hate VB.NET's array syntax).

The code below works in 2 parts:
- The first part (LoadMapperTableAsync) is only concerned with loading data from your Mapper_table into program-memory (into a List, where it uses ValueTuples to represent your table data).
- The second part (ConvertTo2DArray) converts the loaded data into a 2D array of colors.
It does this with 2 consecutive loops (for O(n) runtime complexity):
- The first loop is in LoadMapperTableAsync as it iterates over the rows from the SqlDataReader.
- The second loop is in ConvertTo2DArray and is simply concerned with populating the array.
  - When you create a new array in .NET, the CLR will use pre-zeroed memory, so all T[] array elements automatically have the value default(T). In this case, the default for FillColor is None, which I assume is what you want.
  - Remember that setting array elements by index is a O(1) operation.
The two loops could be combined, but I strongly recommend against it because then you're mixing dumb and straightforward data loading code with application-logic (aka business-logic or domain-logic) which is generally a bad thing because it makes program maintenance much harder. I appreciate you're new to this, but there are no hard-and-fast rules, but I'll say that with increasing experience you'll get a "feel" for what behaviour belongs in what parts of your program and what parts should be kept separate from other parts.

public enum FillColor {
    None = 0, // It's important that zero represents "empty" or "not set" due to .NET's rules on default values for value-types like enums.
    Red = 1,
    Blue = 2,
    // etc
}

public static async Task< List<( Int32 x, Int32 y, FillColor fill )> > LoadMapperTableAsync()
{
    const String connectionString = "...";

    using( SqlConnection c = new SqlConnection( connectionString ) )
    using( SqlCommand cmd = c.CreateCommand() )
    {
        await c.OpenAsync().ConfigureAwait(false);

        cmd.CommandText = "SELECT x, y, fill FROM Mapper_Table"; // NEVER EVER USE `SELECT * FROM` in application code!!!!!! Always explicitly name your columns!
        
        using( SqlDataReader rdr = await cmd.ExecuteReaderAsync().ConfigureAwait(false) )
        {
            List<( Int32 x, Int32 y, FillColor fill )> list = new List<( Int32 x, Int32 y, FillColor fill )>();
            
            while( await rdr.ReadAsync().ConfigureAwait(false) )
            {
                Int32 x    = rdr.GetInt32( 0 ); // Add `rdr.IsDBNull()` guards, if necessary!
                Int32 y    = rdr.GetInt32( 1 );
                Int32 fill = rdr.GetInt32( 2 );

                list.Add( ( x, y, (FillColor)fill ) );
            }

            return list;
        }
    }
}

public static FillColor[,] ConvertTo2DArray( IEnumerable< ( Int32 x, Int32 y, FillColor fill ) > values )
{
    const Int32 MAP_WIDTH  = 100;
    const Int32 MAP_HEIGHT = 100;

    FillColor[,] map = new FillColor[ MAP_HEIGHT, MAP_WIDTH ];

    // Do note that 2D arrays in .NET are slooooow: https://stackoverflow.com/questions/468832/why-are-multi-dimensional-arrays-in-net-slower-than-normal-arrays

    foreach( ( Int32 x, Int32 y, FillColor fill ) in values )
    {
        // Skip-over invalid data:
        if( x < 0 || x > MAP_WIDTH  ) continue;
        if( y < 0 || y > MAP_HEIGHT ) continue;

        map[ y, x ] = fill;
    }

    return map;
}

public static async Task LoadAsync()
{
    List<( Int32 x, Int32 y, FillColor fill )> list = await LoadMapperTableAsync().ConfigureAwait(false);

    return ConvertTo2DArray( list );
}

Is there any obvious reason why my loop is not working as it should be?

Answers (2)

Related Questions