patterncsharpMinor
C# Messy Function - Refactoring
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.
Thanks,
Alex
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
to,
I would save reused values in scoped variables:
I would consider refactoring the foreach
into a Linq select:
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.