Pass data from database table to an array

Question

i am having some trouble trying to stock one row (Id) of my table (contas) to my array (ids[ ]), according to the requisites (posicao like 'Saúde', convenio like convenio and i can't have any duplicated Id's). The variables 'e' and 'i' are merely accountants. I'm kinda new to coding so please don't judge me hard. Also it's my first post on this site + I don't know english very well.

This is my code:

// Here I select the number of Id's that are compatible with my requisites, so far so ok
cmd = new SqlCommand("select COUNT(Id) from contas where posicao like 'Saúde' and convenio like '" +convenio+"'", con);
cmd.Parameters.AddWithValue("@Id", Id);
numeroidentista = (cmd.ExecuteScalar()).ToString();

// Here I create my arrays to store the data
int[] ids = new int[Convert.ToInt32(numeroidentista)];
string[] idst = new string[Convert.ToInt32(numeroidentista)];
string[] inst = new string[Convert.ToInt32(numeroidentista)];

// And here I tryied so hard and it doesn't even matter
while (e < Convert.ToInt32(numeroidentista))
{
    SqlCommand cmdao = new SqlCommand(inst[i].ToString(), con);
    inst[i] = "SELECT Id FROM contas where posicao like 'Saúde' and convenio like '" + convenio + "' and Id > '" + ids[i] + "'";
    SqlDataReader reader = cmdao.ExecuteReader();
    if (reader.Read())
    {
        while (i < Convert.ToInt32(numeroidentista))
        {
            idst[i] = reader["Id"].ToString();
            ids[i] = Convert.ToInt32(idst[i]);
            i++;
        }
    }
    e++;
    reader.Close();
}

You said "posicao = 'Saúde', convenio = a", but your code shows `posicao like 'Saúde' and convenio like`. Was this intentional? The `LIKE` keyword is not the same as =. See [this post](https://stackoverflow.com/questions/1504990/whats-the-difference-between-like-and-in-sql) for a full explanation. — Timothy G., Aug 19 '18 at 18:05
My mistake, alredy fixed this detail. But in the code is really "like". — Victor H., Aug 19 '18 at 18:12
You're adding a parameter `@Id` but never use it in your query, nor ever assign anything to it. Also try it with `convenio` = `' OR ''='`. — Dour High Arch, Aug 19 '18 at 18:18
You've received some good guidance here to help you get your code to work. For learning purposes, I'd suggest finding a way to turn all of your queries into one. Right now, you're sending many queries to the database, all hitting the same table, doing very similar things. In other words, what if you pulled in all of the rows that might be relevant once at the beginning, then you turn that collection into the final resultset (and I'd suggest doing it with objects, so a List). Other things: SqlConnection, SqlCommand, and SqlDataReader are all IDisposable, so wrap them in usings. — Kevin Fichter, Aug 19 '18 at 18:50

Olivier Jacot-Descombes · Accepted Answer · 2018-08-20T13:08:27.410

There are several problems with your code; however, the main one is, that you are using inst[i] before assigning it a value:

SqlCommand cmdao = new SqlCommand(inst[i].ToString(), con); // Using inst[i] here.
inst[i] = "SELECT Id FROM contas where posicao like 'Saúde' and convenio like '" + convenio +
     "' and Id > '" + ids[i] + "'"; // But assigning it here.

Swap the lines

inst[i] = "SELECT Id FROM contas where posicao like 'Saúde' and convenio like '" + convenio +
    "' and Id > '" + ids[i] + "'";
SqlCommand cmdao = new SqlCommand(inst[i].ToString(), con);

And since inst[] is a string array, no conversion to string is required:

SqlCommand cmdao = new SqlCommand(inst[i], con);

You have other superfluous conversions. You convert the COUNT(id), which is already an int to a string, just to convert it to and int again later:

//Not shown
string numeroidentista;

numeroidentista = (cmd.ExecuteScalar()).ToString();
int[] ids = new int[Convert.ToInt32(numeroidentista)];

Change it to

int numeroidentista = (int)cmd.ExecuteScalar();
int[] ids = new int[numeroidentista];

Instead of fixed size arrays I would use variable size lists (List<T>). This would make the first SELECT COUNT(Id) superfluous as well.

Another question is what type is Id in the table? You sore it in an int array, but the SQL surrounds it with apostrophes, suggesting that it has a text type. It should be and int in the table as well, in which case there should be no apostrophes in the SQL: ... and Id > " + ids[i];. You are also using ids[i] before assigning it a value.

It would be preferred to use command parameters everywhere instead of string concatenation. But since you are storing the resulting SQL, you might want to keep it this way for later reference.

Why are you using 3 different arrays. By using a class string with the 3 fields, the code would become easier. Using this class

public class ContaInfo
{
    public int Id { get; set; }
    public string IdString { get; set; }
    public string Instruction { get; set; }
}

You could declare a list like

var contas = new List<ContaInfo>();

and then add items with

contas.Add(new ContaInfo { Id = ..., IsString = ..., Instruction = ...});

Finally, using-statements automatically close and dispose resources.

I tried to rewrite the code; however, I do not understand the logic behind all this. You are creating an array of a fixed size, but then you have 2 nested loops, potentially creating more entries than the size of the arrays.

Thank you! And I laughed really hard when I realized the superfluous conversions. God damn it self! — Victor H., Aug 19 '18 at 18:43

score 1 · Answer 2 · answered Aug 19 '18 at 18:24

1- You have several potential issues in your code. When you are doing the conversions, you are trusting the type in the string be convertable to your desired type for example int. Examples are Convert.ToInt32(numeroidentista) , Convert.ToInt32(idst[i]) , the recommended way is to do these using TryParse, for example :

if(!Int32.TryParse(numeroidentista, out int numeroidentistaInt))
    {
        Console.WriteLine($"Error converting {numeroidentista} to int value");
        return;
    }

2- Also for efficiency, do the conversions only once, you have repeated Convert.ToInt32(numeroidentista) 5 times . Just do it once and before your while loop.

3- And finally as pointed in the comment above, you have not initialized your inst array before using it.

4- As another tip, try to use $"" convention when forming a string using constants and variables. For example you can replace your first string with $"select COUNT(Id) from contas where posicao like 'Saúde' and convenio like '{convenio}'"

Pass data from database table to an array

2 Answers2