HiveBrain v1.2.0
Get Started
← Back to all entries
patterncsharpMinor

C# Messy Function - Refactoring

Submitted by: @import:stackexchange-codereview··
0
Viewed 0 times
refactoringfunctionmessy

Problem

Just started to clean up an old code base.
I'm interested in your perspective and ways that you would change the following function.

private void MarkSelectedRow()
    {
        Object obj = Bankkonto1.Value;
        if ((obj != null) && (myBankkonten.Contains(obj.ToString())))
        {
            foreach (UltraGridRow myRow in Bankkonto1.Rows)
            {
                if (myRow.Cells[STR_Id].Text != obj.ToString()) 
                    continue;

                myRow.Selected = true;
                try
                {
                    hV_OPPanel1.SetBankListe(myBankkonten, Bankkonto1.SelectedRow.Index);
                }
                catch
                {}
                return;
            }
            if (Bankkonto1.Rows.Count > 0)
                Bankkonto1.Rows[0].Selected = true;
        }
        else
            SelectDefaultBank();

        try
        {
            Bankkonto2.Rows[Bankkonto1.SelectedRow.Index].Selected = true;
            BankkontoS.Rows[Bankkonto1.SelectedRow.Index].Selected = true;
            AutoBank.Rows[Bankkonto1.SelectedRow.Index].Selected = true;
            hV_OPPanel1.SetBankListe(myBankkonten, Bankkonto1.SelectedRow.Index);
        }
        catch
        {}
    }

    public void SetBankListe(BankAccountList bankliste, int SelectedIndex)
    {

        if (OPBankkonto.DataSource != bankliste)
            OPBankkonto.SetDataBinding(bankliste, "");

        OPBankkonto.ValueMember = "Id";
        OPBankkonto.DisplayMember = "DisplayText";
        OPBankkonto.DataBind();
        OPBankkonto.Rows[SelectedIndex].Selected = true;
    }


Thanks,
Alex

Solution

I would consider inverting the first if to make it simpler and quicker to be aware of the else.

I strongly suggest always leaving something inside a catch - if you have nothing else to put there at least leave a comment.

I would change

if (Bankkonto1.Rows.Count > 0)


to, using System.Linq; :

if (Bankkonto1.Rows.Any())


I would save reused values in scoped variables:

string objValue = obj.ToString();
...
int selectedRow = Bankkonto1.SelectedRow.Index;


I would consider refactoring the foreach

foreach (UltraGridRow myRow in Bankkonto1.Rows)
        {
            if (myRow.Cells[STR_Id].Text != obj.ToString()) 
                continue;


into a Linq select:

var row = Bankkonto1.Rows.FirstOrdefault( r => r.Cells[STR_Id].Text == objValue);
        if(row != null) {

Code Snippets

if (Bankkonto1.Rows.Count > 0)
if (Bankkonto1.Rows.Any())
string objValue = obj.ToString();
...
int selectedRow = Bankkonto1.SelectedRow.Index;
foreach (UltraGridRow myRow in Bankkonto1.Rows)
        {
            if (myRow.Cells[STR_Id].Text != obj.ToString()) 
                continue;
var row = Bankkonto1.Rows.FirstOrdefault( r => r.Cells[STR_Id].Text == objValue);
        if(row != null) {

Context

StackExchange Code Review Q#6027, answer score: 5

Revisions (0)

No revisions yet.