Is it wrong to put that amount of code in a button click event?

1

Is it wrong to put that amount of code into a button click event?

private void CPeBTNSalvar_Click(object sender, EventArgs e)
    {
        try
        {
            if (CPeTBCodigo.Text == "")
            {
                throw new ExcecaoModificada("Determine um codigo para o pedido!");
            }

            int codigo = int.Parse(CPeTBCodigo.Text);
            int ano = CpeData.Value.Year;
            int mes = CpeData.Value.Month;
            int dia = CpeData.Value.Day;
            bool semPedido = true;

            Pedido p = new Pedido(codigo, ano, mes, dia);

            foreach (DataGridViewRow x in dG_CPeListarProdutos.Rows)
            {
                if (Convert.ToInt32(x.Cells[4].Value) > 0)
                {
                    ItemPedido item = new ItemPedido(FormularioPrincipal.lProduto[x.Index], Convert.ToInt32(x.Cells[4].Value), Convert.ToDouble(x.Cells[3].Value));
                    p.itens.Add(item);
                    semPedido = false;
                    x.Cells[3].Value = 0;

                    x.Cells[4].Value = 0;
                }
                else
                {
                    x.Cells[3].Value = 0;
                }
            }
            if (semPedido == true)
            {
                throw new ExcecaoModificada("Escolha algum produto!");
            }
            else
            {
                CPeLabelAlerta.Visible = true;
                CPeLabelAlerta.Text = "Novo pedido adicionado!";
                CPeLabelAlerta.ForeColor = Color.DarkSlateGray;
                FormularioPrincipal.lPedido.Add(p);
            }
        }
        catch (ExcecaoModificada erro)
        {
            CPeLabelAlerta.Visible = true;
            CPeLabelAlerta.Text = erro.Message;
            CPeLabelAlerta.ForeColor = Color.Red;
        }
        catch (Exception)
        {
            CPeLabelAlerta.Visible = true;
            CPeLabelAlerta.Text = "Coloque apenas numeros em código!";
        }
    }

If you think it's not wrong, quote something that would not do as I did.

    
asked by anonymous 22.07.2018 / 22:08

2 answers

4

You should have only the most obvious code to execute when the button is clicked.

Part of the code does not seem to have to do with pushing the button itself, so it should be delegated to other methods, such as validating and preparing other parts of the screen. It also depends a lot on the rest of the code to evaluate. In general they will be private or even belonging to another class (in the case of the validation itself which is business rule and not screen rule).

In fact it seems to have much more important errors in this code, like Pedido which is quite weird, the form of data conversion is prone to errors, the use of exceptions is wrong in every sense. It's just not easy to pack without fully knowing the problem. The problem seems to be structural, so you would not have to fix it, you would have to fix the entire application.

    
22.07.2018 / 22:41
1

There is a lot of redundancy in your code. Will the CPeBTNSalvar_Click event request the registration of something in the database or file? It seems that he is only looking for the data of DataGridView and adding in his class, and as @Miero said, he should have only the necessary code. Let the names be clearer and legible too, just use a prefix if you have some worthwhile benefit.

Validation of numeric data

In your code, just one method to validate

if (int.TryParse(CPeTBCodigo.Text, out var resultado)){ ... }

You do not need to validate more than once as CPeTBCodigo.Text == "" and int.Parse(CPeTBCodigo.Text); , as it is in your code.

Exceptions

You can learn the basics of how to use exceptions here , always try to avoid them. Unnecessary exceptions can get in the way of tracking down an error or impairing application performance. And in your case you use exceptions to treat data that comes from the user, there is no need to create exceptions for this.

Business rules

However, if you want, you can use the DataAnnotations to define the meta data for controlling the data held in the object. Next you% w / o% to describe the context of the validation and% w / o of% to get the result of the validation and the error messages.

Small example illustration

The example below illustrates how to use the features mentioned above, see:

using System.ComponentModel.DataAnnotations; // Tem que adicionar esta referencia ao seu projeto.

public class Produto
{
    public long Id { get; set; }

    [Required(ErrorMessage = "Este campo é obrigatório.")]
    public string Descricao { get; set; }

    [Range(1.0, Double.MaxValue, ErrorMessage = "Oops... o preço não pode ser 0 (zero) ou menor.")]
    public decimal Preco { get; set; }


    public void Validate()
    {
        Produto produto = this;
        ValidationContext context = new ValidationContext(produto, null, null);
        IList<ValidationResult> erros = new List<ValidationResult>();

        if (!Validator.TryValidateObject(cliente, context, erros, true))
        {
            foreach (ValidationResult resultado in erros)
            {
                MinhaListaDeErros.Add(resultado.ErrorMessage);
            }
        }   

        if (!MinhaOutraValidacao()) MinhaListaDeErros.Add("Minha outra mensagem de erro.");
    }   
}

The above example is to show a path, there is a lot that needs to be improved, so be careful, and consider the context and the need for your application.

Learn more about DataAnnotations .

More about field validation .

    
24.07.2018 / 00:09