Anything wrong with the way I use "break"s in my loops?

  • Thread starter Thread starter Curious
  • Start date Start date
C

Curious

I have a simple method that does a search. Details below:

"mBenchmarkPriceList" is an ArrayList with each element being of
"BenchmarkPrice" type. There are three members in "BenchmarkPrice":
Ticker, Side, and AggregateLists.
The member, "AggregateLists", however, is an ArrayList with each
element being a "BMarkUnit" type. "BMarkUnit" contains two members,
ListName and OriginalWeight.

The method, "GetOriginalWeight", searches through the entire
"mBenchmarkPriceList". If a match of Ticker, Side, and also ListName
is found, it will return "OriginalWeight".

In order to be efficient, once a match is found, I "break" both loops
and return the value for "OriginalWeight". However, it seems that an
incorrect value is returned most of the time. Is there anything wrong
with the way I use either "break"?

Thanks!

double GetOriginalWeight ( string list, string ticker, string
side )
{
double origWeight = 0.0;
bool found = false;

foreach (BenchmarkPrice bp in mBenchmarkPriceList)
{

if ( bp.Ticker == ticker &&
bp.Side == side || ( side == "Buy" && bp.Side ==
"Cover" ) || ( side == "Sell" && bp.Side == "Short" ))
{
foreach (BMarkUnit bu in bp.AggregateLists)
{
if ( bu.ListName == list )
{
origWeight = bu.OriginalWeight;

found = true;
break;
}
}
}

if (found)
{
break;
}
}

return origWeight;
}
 
There isn't anything logically wrong the way you use "break" twice to get
out of the 2 loops, but a few lines of tedious code. You can make the code a
bit more concise by immediately returning the found value in the inner loop:

foreach (...)
{
....
foreach (BMarkUnit bu in bp.AggregateLists)
{
if ( bu.ListName == list )
{
return bu.OriginalWeight;
}
}
}

As for execution efficientcy, I do not think it makes noticeable difference.
 
Curious said:
I have a simple method that does a search. Details below:

"mBenchmarkPriceList" is an ArrayList with each element being of
"BenchmarkPrice" type. There are three members in "BenchmarkPrice":
Ticker, Side, and AggregateLists.
The member, "AggregateLists", however, is an ArrayList with each
element being a "BMarkUnit" type. "BMarkUnit" contains two members,
ListName and OriginalWeight.

The method, "GetOriginalWeight", searches through the entire
"mBenchmarkPriceList". If a match of Ticker, Side, and also ListName
is found, it will return "OriginalWeight".

In order to be efficient, once a match is found, I "break" both loops
and return the value for "OriginalWeight". However, it seems that an
incorrect value is returned most of the time. Is there anything wrong
with the way I use either "break"?

Thanks!

double GetOriginalWeight ( string list, string ticker, string
side )
{
double origWeight = 0.0;
bool found = false;

foreach (BenchmarkPrice bp in mBenchmarkPriceList)
{

if ( bp.Ticker == ticker &&
bp.Side == side || ( side == "Buy" && bp.Side ==
"Cover" ) || ( side == "Sell" && bp.Side == "Short" ))
{
foreach (BMarkUnit bu in bp.AggregateLists)
{
if ( bu.ListName == list )
{
origWeight = bu.OriginalWeight;

found = true;
break;
}
}
}

if (found)
{
break;
}
}

return origWeight;
}
Click to expand...


I don't know the logic of your matching, but it would seem that your
complicated if () statement is the most likely reason you would get the
wrong answer. I would double check that it is doing what you expect in the
debugger or a test call.
 
I have a simple method that does a search. Details below:

"mBenchmarkPriceList" is an ArrayList with each element being of
"BenchmarkPrice" type. There are three members in "BenchmarkPrice":
Ticker, Side, and AggregateLists.
The member, "AggregateLists", however, is an ArrayList with each
element being a "BMarkUnit" type. "BMarkUnit" contains two members,
ListName and OriginalWeight.

The method, "GetOriginalWeight", searches through the entire
"mBenchmarkPriceList". If a match of Ticker, Side, and also ListName
is found, it will return "OriginalWeight".

In order to be efficient, once a match is found, I "break" both loops
and return the value for "OriginalWeight". However, it seems that an
incorrect value is returned most of the time. Is there anything wrong
with the way I use either "break"?

Thanks!

double GetOriginalWeight ( string list, string ticker, string
side )
{
double origWeight = 0.0;
bool found = false;

foreach (BenchmarkPrice bp in mBenchmarkPriceList)
{

if ( bp.Ticker == ticker &&
bp.Side == side || ( side == "Buy" && bp.Side ==
"Cover" ) || ( side == "Sell" && bp.Side == "Short" ))
{
foreach (BMarkUnit bu in bp.AggregateLists)
{
if ( bu.ListName == list )
{
origWeight = bu.OriginalWeight;

found = true;
break;
}
}
}

if (found)
{
break;
}
}

return origWeight;
}
Click to expand...

The problem almost certainly is not anything wrong with how you use
break, but with the if statement:
if ( bp.Ticker == ticker &&
bp.Side == side || ( side == "Buy" && bp.Side ==
"Cover" ) || ( side == "Sell" && bp.Side == "Short" ))
Click to expand...

Note that you are mixing && with || without parentheses (the first
two). Probably the precedence order is not what you expect.
 
I have a simple method that does a search. Details below:

"mBenchmarkPriceList" is an ArrayList with each element being of
"BenchmarkPrice" type. There are three members in "BenchmarkPrice":
Ticker, Side, and AggregateLists.
The member, "AggregateLists", however, is an ArrayList with each
element being a "BMarkUnit" type. "BMarkUnit" contains two members,
ListName and OriginalWeight.

The method, "GetOriginalWeight", searches through the entire
"mBenchmarkPriceList". If a match of Ticker, Side, and also ListName
is found, it will return "OriginalWeight".

In order to be efficient, once a match is found, I "break" both loops
and return the value for "OriginalWeight". However, it seems that an
incorrect value is returned most of the time. Is there anything wrong
with the way I use either "break"?

Thanks!

double GetOriginalWeight ( string list, string ticker, string
side )
{
double origWeight = 0.0;
bool found = false;

foreach (BenchmarkPrice bp in mBenchmarkPriceList)
{

if ( bp.Ticker == ticker &&
bp.Side == side || ( side == "Buy" && bp.Side ==
"Cover" ) || ( side == "Sell" && bp.Side == "Short" ))
{
foreach (BMarkUnit bu in bp.AggregateLists)
{
if ( bu.ListName == list )
{
origWeight = bu.OriginalWeight;

found = true;
break;
}
}
}

if (found)
{
break;
}
}

return origWeight;
}
Click to expand...

One more thing. This is the kind of problem that you should have been
able to figure out very quickly using the debugger.
 
Curious,

I would set the code in a method and simple set a "return" instead of a
"break" as the match is found and remove the boolean and the evaluating of
that.

Cor
 
Curious said:
In order to be efficient, once a match is found, I "break" both loops
and return the value for "OriginalWeight". However, it seems that an
incorrect value is returned most of the time. Is there anything wrong
with the way I use either "break"?
Click to expand...

No, there is nothing wrong with the breaks, but probably with your
condition:
if ( bp.Ticker == ticker &&
bp.Side == side || ( side == "Buy" && bp.Side ==
"Cover" ) || ( side == "Sell" && bp.Side == "Short" ))
Click to expand...

This evaluates as:

if (( bp.Ticker == ticker && bp.Side == side ) || ( side == "Buy" &&
bp.Side == "Cover" ) || ( side == "Sell" && bp.Side == "Short" ))

I think that what you want is:

if ( bp.Ticker == ticker && ( bp.Side == side || ( side == "Buy" &&
bp.Side == "Cover" ) || ( side == "Sell" && bp.Side == "Short" )))
 
No, there is nothing wrong with the breaks, but probably with your
condition:


This evaluates as:

if (( bp.Ticker == ticker && bp.Side == side ) || ( side == "Buy" &&
bp.Side == "Cover" ) || ( side == "Sell" && bp.Side == "Short" ))

I think that what you want is:

if ( bp.Ticker == ticker && ( bp.Side == side || ( side == "Buy" &&
bp.Side == "Cover" ) || ( side == "Sell" && bp.Side == "Short" )))
Click to expand...

Thanks Goran! You're right. I need (bp.Ticker == ticker ) WITH any of
the other conditions. I wasn't careful.
 
Curious,

I would set the code in a method and simple set a "return" instead of a
"break" as the match is found and remove the boolean and the evaluating of
that.

Cor














- Show quoted text -
Click to expand...

I'm afraid simply returning from inside a loop is not a clean way to
exit the loops?
 
I wasn't able to use the debugger because it's a service project and
this portion of code is executed on a timer. It'll time out if I pause
to take a look at the info in the debugger.
 
Jack,

You're right. My if statement is all messed up. I corrected it as
below and it works:

if ( bp.Ticker == ticker &&
((bp.Side == side) || ( side == "Buy" && bp.Side
== "Cover" ) || ( side == "Sell" && bp.Side == "Short" )))

Thanks!
 
Mike,

You're right. My if statement was screwy. I've changed it to the
following and it works now:

if ( bp.Ticker == ticker &&
((bp.Side == side) || ( side == "Buy" && bp.Side
== "Cover" ) || ( side == "Sell" && bp.Side == "Short" )))
 
Norman,

Now I return the value as soon as there is a match, and it works.
Thanks for the advice!
 
Curious said:
I'm afraid simply returning from inside a loop is not a clean way to
exit the loops?
Click to expand...

Yes, you are basically right, the cleanest way is to keep a single exit
point from the method.

However, if the method is small it's usually acceptable to have two exit
points if it makes the code significantly simpler.
 
I can't believe that noone has mentioned that this function is in dire need
of refactoring. Personally, I would have tried something like the following


double GetOriginalWeight(string list, string ticker, string side)
{
List<string> list = new List<string>();
list.AddRange(mBenchmarkPriceList);

var myMarkUnit = (from benchmarkPrice in list
where ((benchmarkPrice.Ticker == ticker && benchmarkPrice.Side
== side)
|| (side == "Buy" && benchmarkPrice.Side == "Cover")
|| (side == "Sell" && benchmarkPrice.Side == "Short"))
from markUnit in benchmarkPrice.AggregateLists
where markUnit.ListName == list
select markUnit).FirstOrDefault();

if (myMarkUnit != null)
return myMarkUnit.OriginalWeight;

return 0.0d;
}

Which negates the need for any breaking at all. Also prior to the
availability of Linq, I would have done something similar to the following
....

double GetOriginalWeight(string list, string ticker, string side)
{
double origWeight = 0.0;
BenchmarkPrice bp = RetrievePertinentBenchmarkPrice(ticker,
side);
BMarkUnit bu = RetrievePertinentBMarkUnit(list, bp);
if(bu != null)
origWeight = bu.OriginalWeight;

return origWeight;
}

BMarkUnit RetrievePertinentBMarkUnit(string list, BenchmarkPrice bp)
{
if (bp == null)
return null;

foreach (BMarkUnit bu in bp.AggregateLists)
if (bu.ListName == list)
return bu;

return null;
}

BenchmarkPrice RetrievePertinentBenchmarkPrice(string ticker, string
side)
{
foreach (BenchmarkPrice bp in mBenchmarkPriceList)
if (IsThisTheBenchmarkPriceIAmInterstedIn(bp))
return bp;

return null;
}

bool IsThisTheBenchmarkPriceIAmInterstedIn(BenchmarkPrice bp)
{
return (bp.Ticker == ticker && bp.Side == side || (side == "Buy"
&& bp.Side == "Cover") ||
(side == "Sell" && bp.Side == "Short"));
}
 
Back
Top