Is this code appropriate or a bad practice?

Asked

Viewed 51 times

0

Good afternoon, I am a beginner in programming and in case my problem would be with C# and Windows Forms. I have a screen for reports of a PDV system, and in this report I can select the daily, monthly and annual options (the choice is made through a ComboBox). However, for each of these options I also have 'sub-options', for example, monthly type reports can be to list how much was sold of each product, or to list all sales, for example (this choice is also made through a CombobBox), given that will be launched in a DataGridView. At first, I started using several ifs, but this resulted in a huge chain of selection loops. Something like this:

if(cbxTime.SelectedIndex == 0)//Relatório diário
{
    if(cbxSubOption.SelectedIndex == 0)//Relatório diário de produtos
    {
        //Pesquisava no banco e jogava dentro de uma DataGridView
    }
    else if(cbxSubOption.SelectedIndex == 1)//Relatório diário de vendas
    {
        //Pesquisava no banco e jogava dentro de uma DataGridView
    }
    else
    {  }
}
else if(cbxTime.SelectedIndex == 1)//Relatórios mensais
{
    //uma cadeia de ifs semelhante à do primeiro if.
}
else//Relatórios Anuais
{  }

As I didn’t find it very viable, I tried to replace this solution with something more dynamic, but I don’t know if it was correct. My idea was to create a vector of Dictionary, more specifically a vector of three positions, each representing one of the time bands. Each Dictionary was composed of an index that would be the index of the sub-Combobox and a action, a reference to a non-return function that would execute the code I needed. So I created a function for each of the possible combinations of ComboBox and played in this variable. Within each of them, I performed the actions necessary to play the dice in the DataGridView, something like this:

private Dictionary<int, Action>[] dataGridActions;

public void Form1()//No construtor
{
    dataGridActions = new Dictionary<int, Action>[3]
        {
            new Dictionary<int, Action>()
            {
                { 0, ListDailyProducts},
                { 1, ListDailySales},
                { 2, x},
            },
            new Dictionary<int, Action>()
            {
                { 0, x},
                { 1, x},
                { 2, x},
            },
            new Dictionary<int, Action>()
            {
                { 0, x},
                { 1, x},
                { 2, x},
            },
        };
}

private void ListDailySales()
{
    //Fazia a devida ação
}
private void ListDailyProducts()
{
    //Fazia a devida ação
}

At the time of invoking:

int time = cbxTime.SelectedIndex;
int option = cbxSubOption.SelectedIndex;
gridActions[time][option].Invoke();

It’s all working perfectly, but the question is, is it correct what I did here? All this is just in the form. I kind of read about Strategy to try to do, but I know very well that I didn’t use any of that very much in practice. I would like to know if there is a more correct way to do it, and where I should seek solutions for cases like this in the future, thank you from now.

  • 1

    It would be interesting for you to create Enums for these fixed values you are using in your logic. So whoever looks at the code will be able to better understand what is written.

  • Thanks @Leandroangelo, I’ll do it

No answers

Browser other questions tagged

You are not signed in. Login or sign up in order to post.