5
\$\begingroup\$

I am a bit new to programming. I have some edits on my form, and for each edit completed it will have to do a procedure.

Currently, I use this code that works perfectly, but I am looking for a better way to accomplish this task. If I add another edit not have to repeat all the code.

string rgxExpr = @"(\w+)\s([\w\s]*)"; // Pega primeiro nome
string rgxExpr2 = @"\b(\w{1})"; // Abreviação
string s1, s2, s3, s4,s5;

/* EDIT 1 */
Match match1 = Regex.Match(edtAutor1.Text, rgxExpr);
Match match2 = Regex.Match(match1.Groups[1].ToString(), rgxExpr2);
s1 = match1.Groups[2].ToString().ToUpper() + ", " + match2.Groups[1].ToString().ToUpper();
// Apenas um autor
s4 = match1.Groups[2].ToString().ToUpper() + ", " + match1.Groups[1].ToString().ToUpper();
/* EDIT 2 */
Match match11 = Regex.Match(edtAutor2.Text, rgxExpr);
Match match22 = Regex.Match(match11.Groups[1].ToString(), rgxExpr2);
s2 = match11.Groups[2].ToString().ToUpper() + ", " + match22.Groups[1].ToString().ToUpper();
/* EDIT 3 */
Match match111 = Regex.Match(edtAutor3.Text, rgxExpr);
Match match222 = Regex.Match(match111.Groups[1].ToString(), rgxExpr2);
s3 = match111.Groups[2].ToString().ToUpper() + ", " + match222.Groups[1].ToString().ToUpper();
/* EDIT 4 */
Match match1111 = Regex.Match(edtAutor4.Text, rgxExpr);
Match match2222 = Regex.Match(match1111.Groups[1].ToString(), rgxExpr2);
s5 = match1111.Groups[2].ToString().ToUpper() + ", " + match2222.Groups[1].ToString().ToUpper();

if (radioButton1.Checked == true)
{
    if (edtAutor1.Text != "")
    {
        /* Bibliográfia - 1 AUTOR */
        if (string.IsNullOrWhiteSpace(edtAutor2.Text) && string.IsNullOrWhiteSpace(edtAutor3.Text))
        {
            addbb.dataGridView1.Rows.Add(s4+". ", abntrTitulo.Text+"." , abntrEdicao.Text +" Ed. ", abntrLocal.Text + ": ", abntrEditor.Text + ", ",
            abntrAno.Text + ". ", abntrPaginas.Text +"p.");
        }

        /* Bibliográfia - 2 AUTORES */
        if (string.IsNullOrWhiteSpace(edtAutor3.Text))
        {
            if (edtAutor2.Text != "")
            {
                addbb.dataGridView1.Rows.Add(s1 + "; " + s2 + ". ", abntrTitulo.Text + ".", abntrEdicao.Text + " Ed. ", abntrLocal.Text + ": ", abntrEditor.Text + ", ",
            abntrAno.Text + ". ", abntrPaginas.Text + "p.");
            }
        }

        /* Bibliográfia - 3 AUTORES */
        if (string.IsNullOrWhiteSpace(edtAutor4.Text))
        {
            if (edtAutor1.Text != "" & edtAutor2.Text != "" & edtAutor3.Text != "")
            {
                addbb.dataGridView1.Rows.Add(s1 + "; " + s2 + "; " + s3 + ". ", abntrTitulo.Text + ".", abntrEdicao.Text + " Ed. ", abntrLocal.Text + ": ", abntrEditor.Text + ", ",
            abntrAno.Text + ". ", abntrPaginas.Text + "p.");
            }
        }

        /* Bibliográfia - 4 AUTORES */
        if (edtAutor1.Text != "" & edtAutor2.Text != "" & edtAutor3.Text != "" & edtAutor4.Text != "")
        {
            addbb.dataGridView1.Rows.Add(s1 + "; " + s2 + "; " + s3 + ";" + s5 + ". ", abntrTitulo.Text + ".", abntrEdicao.Text + " Ed. ", abntrLocal.Text + ": ", abntrEditor.Text + ", ",
            abntrAno.Text + ". ", abntrPaginas.Text + "p.");
        }
    }
    else
    {
        MessageBox.Show("Você precisa preencher todos os campos obrigátorios (*). Por favor, verifique se todos os campos estão preenchidos corretamente!");
    }


}

The code uses regex and separates the words of EDIT, reverses the order and abbreviates.

\$\endgroup\$
  • \$\begingroup\$ When you want to add another edit do you have to add another edtAutorX textbox? \$\endgroup\$ – Paweł Hemperek Oct 8 '16 at 8:26
  • \$\begingroup\$ Lot of code is run that is only used if radioButton1.Checked == true \$\endgroup\$ – paparazzo Oct 8 '16 at 12:49
  • \$\begingroup\$ There are lot of things wrong in your code. Improving only small parts of it doesn't make much sense. Besides it's difficult to unterstand it as this is only a short snippet. We don't know how the parsed text looks like - I'm pretty sure that you don't need two patterns for it. This is really a mess ;-) Can you add some more information and maybe more code? With examples? \$\endgroup\$ – t3chb0t Oct 10 '16 at 6:33
2
\$\begingroup\$

As I confirmed in comments, this part

Match match1 = Regex.Match(edtAutor1.Text, rgxExpr);
Match match2 = Regex.Match(match1.Groups[1].ToString(), rgxExpr2);
s1 = match1.Groups[2].ToString().ToUpper() + ", " + match2.Groups[1].ToString().ToUpper();

is really unnecessary. Instead you should replace it with simple function

string GetAuthor(string author)
{
    string[] splittedAuthor = author.Split(' ');
    string firstName = splittedAuthor.First();
    string lastName = splittedAuthor.Skip(1).First();

    // In case you're not using C# 6 use following line
    // return string.Format("{0}, {1}", lastName, firstName[0]);
    return $"{lastName} {firstName[0]}";
}

As @tinstaafl mentioned in his answer - you could add all your textboxes to simple list and then process them in foreach loop

List<TextBox> authorsTextboxes = new List<TextBox>()
{
    edtAutor1,
    edtAutor2,
    edtAutor3,
    edtAutor4
}

Then variables s1, s2, s3, s5 could be replaced with some other list, where you can store results of GetAuthor function.

List<string> authors = new List<string>();
foreach(TextBox authorTextbox in authorTextboxes)
{
    if(!string.IsNullOrEmpty(authorTextbox.Text))
    {
        authors.Add(GetAuthor(authorTextbox.Text));
    }
}

Then you can generate string you are passing to addbb.dataGridView1.Rows.Add.

string concatenatedAuthors = authors.Aggregate((authors, author) => string.Format("{0}; {1}", authors, author));

Since you have all your authors concatenated you don't need to perform all those checks like if (string.IsNullOrWhiteSpace(edtAutor3.Text)). Instead you can simply do this

if (radioButton1.Checked == true)
{
    if (edtAutor1.Text != "")
    {
        addbb.dataGridView1.Rows.Add(concatenatedAuthors + ". ", abntrTitulo.Text + ".", abntrEdicao.Text + " Ed. ", abntrLocal.Text + ": ", abntrEditor.Text + ", ", abntrAno.Text + ". ", abntrPaginas.Text + "p.");
    }
}
else
{
    MessageBox.Show("Você precisa preencher todos os campos obrigátorios (*). Por favor, verifique se todos os campos estão preenchidos corretamente!");
}
\$\endgroup\$
  • \$\begingroup\$ Thank you man, you helped me to make the most efficient code. Thank you very much.... \$\endgroup\$ – Kawyllainy Vi Oct 10 '16 at 13:47
  • \$\begingroup\$ Well - I'm sure there is much more that could be done to improve efficiency of this code, but it's dealing with duplicated code problem, so that's a start :-). I'm glad I could help! \$\endgroup\$ – Paweł Hemperek Oct 10 '16 at 13:53
2
\$\begingroup\$

It looks to me that you could use a method that returns the appropriate string. Something like the following, which follows the pattern in your code:

static string GetEdit(string txt)
{
    string rgxExpr = @"(\w+)\s([\w\s]*)"; // Pega primeiro nome
    string rgxExpr2 = @"\b(\w{1})"; // Abreviação    
    Match match1 = Regex.Match(txt, rgxExpr);
    Match match2 = Regex.Match(match1.Groups[1].ToString(), rgxExpr2);
    return match1.Groups[2].ToString().ToUpper() + ", " + match2.Groups[1].ToString().ToUpper();
}

You could do something similar with the other duplicated code by using a list of strings that correspond to a list of textboxes. allowing you to add or delete as needed.

\$\endgroup\$
  • \$\begingroup\$ I've been thinking about answering this question by myself and I also spotted, that there is a lot of code duplication. But I noticed something else - is't this whole regex unnecessary? I mean all it does is splitting string by spaces to separate firstname and lastname. The second is simply taking the first letter of firstname. Am I right or I missed something? \$\endgroup\$ – Paweł Hemperek Oct 9 '16 at 9:42
  • \$\begingroup\$ It could be, I'm not very knowledgeable about Regex. From what I've hear, most times it's a toss up whether regex or linq is better. If what you say is true, then just using the first character of the string would be better that using Regex in the second instance. \$\endgroup\$ – tinstaafl Oct 9 '16 at 22:15
  • \$\begingroup\$ @PawelHemperek, you are right :) \$\endgroup\$ – Kawyllainy Vi Oct 10 '16 at 11:12
  • \$\begingroup\$ @KawyllainyVi if so, then I will post my answer later - maybe you'll find it helpful \$\endgroup\$ – Paweł Hemperek Oct 10 '16 at 11:13

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.