Coding style II - O(n) checks instead O(1) or doubling code? [modified]

[Lutoslaw]

What is better:

void AddSharpsAndFlats(StaffVisual staff, Clef clef)
{
int symbolCount = _context.GetSharpCount();
if (symbolCount > 0)
for (int i = 0; i < symbolCount; i++)
{
staff.AddVisual(new SharpVisual
{
Position = clef.SharpPostions[(int)MusicContext.SharpOrder[i]]
});
staff.AddVisual(PlaceHolderVisual.ThinPlaceholder);
}
else if (symbolCount < 0)
for (int i = 0; i < -symbolCount; i++)
{
staff.AddVisual(new FlatVisual
{
Position = clef.FlatPostions[(int)MusicContext.FlatOrder[i]]
});
staff.AddVisual(PlaceHolderVisual.ThinPlaceholder);
}
}

OR: move symbolCount < 0 inside a loop (and use abs(symbolCount))? Then I would have a single loop but I would also have n checks. Note: symbolCount is a value in range {-7,...,-1,0,1,...,7}. EDIT: corrected typo Thanks and

Greetings - Jacek

modified on Tuesday, July 5, 2011 7:50 AM

[Pete OHanlon]

Your logic doesn't work here. You can't have i starting at zero and then being compared to symbolCount when symbolCount is less than zero. The loop won't run.

Forgive your enemies - it messes with their heads

My blog | My articles | MoXAML PowerToys | Mole 2010 - debugging made easier - my favourite utility

[Lutoslaw]

t'was a typo which came out while debugging a moment after I have posted this / forgot to correct. It's done now.

Greetings - Jacek

[Pete OHanlon]

As these are mutually exclusive conditions, I think that having them as separate loops is a perfectly valid construct. As soon as you add the abs(symbolCount) condition in there, you are introducing something that requires thinking about by somebody reviewing your code. While this isn't, necessarily, a bad thing, consider that if this is a commercial project, then at some point, Nervous Nigel the spotty apprentice coder is going to try to do something with the code and end up breaking it because he doesn't understand it. The KISS principal applies.

Forgive your enemies - it messes with their heads

My blog | My articles | MoXAML PowerToys | Mole 2010 - debugging made easier - my favourite utility

[PIEBALDconsult]

I dunno. Maybe squeeze the loops into a common method?

if (symbolCount > 0)
{
AddSymbols ( SharpFactory , clef.SharpPostions , staff , symbolCount ) ;
}
else if (symbolCount < 0)
{
AddSymbols ( FlatFactory , clef.FlatPostions , staff , symbolCount ) ;
}

Just a thought. Otherwise, leave it the way it is.

[BobJanova]

The two loops set off my code style alarms. But having reviewed it, and worked out the domain here, it really is two separate operations, and there's not much there that is common enough to pull out. When it comes down to it you're duplicating four lines:

for(int i = 0; i < symbolCount; i++) {
staff.AddVisual(something);
staff.AddVisual(PlaceHolderVisual.ThinPlaceholder);
}

So I think I would leave it, but with a note (mental or explicit) to watch this space because if you add more key signature related stuff it could get messy and need refactoring. Now you could take the duplication out like this:

void AddSharpsAndFlats(StaffVisual staff, Clef clef)
{
int symbolCount = _context.GetSharpCount();

if(symbolCount != 0){
// I guess this is List<Position> or Position[]
var Positions = symbolCount > 0 ? clef.SharpPositions : clef.FlatPositions;
var Order = symbol.Count > 0 ? MusicContext.SharpOrder : MusicContext.FlatOrder;
VisualCreator creator = symbolCount > 0 ? (p => new SharpVisual(p)) : (p => new FlatVisual(p));

symbolCount = symbolCount > 0 ? symbolCount : -symbolCount; // abs for ints

for (int i = 0; i < symbolCount; i++)
{
  staff.AddVisual(creator( {
    Position = Positions\[(int)Order\[i\]\]
  });
  staff.AddVisual(PlaceHolderVisual.ThinPlaceholder);
}

}
}

... where

delegate VisualCreator(Position position);

(You could also have a VisualFactory that hides that selection away from you.) I'm not convinced that for such a small duplication this is worthwhile.

[kevinnicol]

Nervous Nigel has just made its way into my weekly vocabulary. Describes people I often refer to perfectly. Thank you good sir.

[Pete OHanlon]

You're welcome - I know several of them.

Forgive your enemies - it messes with their heads

My blog | My articles | MoXAML PowerToys | Mole 2010 - debugging made easier - my favourite utility