Loop Abuse

Charlie_Marlow

I told Visual Studio to treat warnings as errors after I finished fixing a few reported bugs, and the following code popped up. Visual studio claims the j++ statement will never execute...

for (int j = 0; j < node.Nodes.Count; j++)
{
   temp = node.Nodes[j].Text + " - " + testString;
   if (temp.Equals(result, StringComparison.CurrentCultureIgnoreCase))
   {
      ...
      if (graphicsLayer.Rows.Count > 0)
      {
         switch (graphicsLayer.FeatureType)
         {
            case ESRI.ArcGIS.ADF.Web.FeatureType.Point:
               ...
               break;
            case ESRI.ArcGIS.ADF.Web.FeatureType.Line:
               ...
               <nested loops>
               ...
               break;
            case ESRI.ArcGIS.ADF.Web.FeatureType.Polygon:
               ...
               <more nested loops>
               ...
               break;
         }// of switch
      }// of graphicslayer rows > 0
   }
   
   // we're done at this point, so go ahead and return.
   return outputDataset;
}

// if we make it to this point then we're DOOM'd

return null;

bstorer

@Charlie Marlow said:

Visual studio claims the j++ statement will never execute...

It won't. What's your point?

Charlie_Marlow

Sorry, I guess I should have pointed out that I realize why it won't execute. I just thought I'd share in the misery of some of the odd code I get to deal with daily.

DaveK1

@Charlie Marlow said:

Sorry, I guess I should have pointed out that I realize why it won't execute. I just thought I'd share in the misery of some of the odd code I get to deal with daily.

I wonder if at some point during the evolution of the code there used to be a 'continue' statement in the switch..case construct? A bit of digging in the VCS history (assuming you have some kind of VCS... this is tdwtf after all, can't take anything for granted) might make some kind of sense of the original intent behind the structure.

Charlie_Marlow

That's how it looked in the original checkin, but I have no way of knowing how many evolutions it made on the developer's machine before he put it into source control. I can say, however, that, from looking at the rest of the code, there should never be more than one entry in node.Nodes. Just seemed to me that this was a really convoluted way of writing an if statement.

AltSysrq

I notice that the iteration variable is j, not i... is this inside ANOTHER loop? I also love the variable name "temp". And WTF is "testString", and do you even know why it's being appended to the node text?

Charlie_Marlow

The more I look at the full method, the more I weep. Yes, it is in another loop. Plus, the first time it drops into the posted loop, it will exit out of the method.As for testString, feast your eyes on this (result is passed into the method):

string testString = string.Empty;
string temp = string.Empty;
int position = node.Text.IndexOf("<a style");

if (position > 0)
{
testString = node.Text.Substring(0, position + 1);
}
else
{
testString = node.Text;
}

temp = node.Nodes[0].Text + " - " + testString;

if (temp.Equals(result, StringComparison.CurrentCultureIgnoreCase))

This is C# code!

danixdefcon5

@Charlie Marlow said:

This is C# code!

I think the Use Of Capitalized Method Names Gives It Away. ;)

Anyway ... it looks like whoever did the "testString" went the extra mile to do

string testString = string.Empty;

instead of

string testString = "";

Nice.

dwilliss

@danixdefcon5 said:

@Charlie Marlow said:
This is C# code!
I think the Use Of Capitalized Method Names Gives It Away. ;)
Anyway ... it looks like whoever did the "testString" went the extra mile to do
string testString = string.Empty;
instead of
string testString = "";
Nice.

I've always wondered: what's the point of string.Empty, anyway?

alegr

@dwilliss said:

I've always wondered: what's the point of string.Empty, anyway?

That all empty strings share the same copy, maybe?

Juifeng

AFAIK, all "" string literals share the same copy, at least in mono.That means it should be exactly the same as string.Empty. Don't know if it's different for MS.NET.

Weng

String.empty, as far as I can tell, is an artifact of the API having to support VB.

I have never seen a VB reference, or a VB coder use anything other than String.empty, which leads me to believe that "" is somehow invalid syntax. I can't be arsed to check, though, because I have to deal with VB for my dayjob right now and have thus banned it in free time.

vyznev

@DaveK said:

I wonder if at some point during the evolution of the code there used to be a 'continue' statement in the switch..case construct?

I'd guess either that, or it might be a simple nesting error: the return statement was meant to go outside the loop, but since there never was more than one node to loop over anyway, nobody noticed that it was misplaced.

(Edit: Actually, I think you're most likely right — the "DOOM'd" comment speaks against my theory.)

pscs

@AltSysrq said:

I notice that the iteration variable is j, not i... is this inside ANOTHER loop? I also love the variable name "temp". And WTF is "testString", and do you even know why it's being appended to the node text?

Actually using 'temp' as a temporary variable name isn't that bad. As long as it only lives for a few lines, it's no worse than using 'i' or 'j' for loop variables. The 'temp' name tells you that it's just temporary, and is probably just used here. (Personally, I'd declare it in the loop as well, to make the small scope more explicit).

dhromed

@pscs said:

Actually using 'temp' as a temporary variable name isn't that bad. As long as it only lives for a few lines, it's no worse than using 'i' or 'j' for loop variables. The 'temp' name tells you that it's just temporary, and is probably just used here.

I slightly disagree.

i and j may seem like nondescript names, but they've become part of the for-loop idiom, and as such have the implicit semantic baggage of "counter integer". There's nothing unknown about i and j.

temp as a var holds absolutely zero meaning, and is just as bad as that result var.

You name your vars according to what you store in them, and nothing else.

Nelle

@dhromed said:

temp as a var holds absolutely zero meaning, and is just as bad as that result var.

I agree "temp" is a pretty non-descriptive name for a var, but why is "result" bad ? I use it often as a local variable in functions for the return value, like

uint32 doSomethingBadToParameters( uint32 a, uint32 b, uint32 c)
{
   uin32 result = 0;

   if (a > b && b < c)
       result = 12;

  if (isPrimeUint32(c))
      result += c;

   return result;
}

Lingerance

@dhromed said:

temp as a var holds absolutely zero meaning

So temporaryMutableCopyOfString, disposableIntermediatelyEditedString, placeHolderForSwaping are better names than temp?

Charlie_Marlow

I am no great fan of result variables, but I would much rather have a result variable and a single exit point from a method than to deal with gotcha return statements burried in nested loops as in the case of the code I posted.

dhromed

@Lingerance said:

So temporaryMutableCopyOfString, disposableIntermediatelyEditedString, placeHolderForSwaping are better names than temp?

Zero points. Your argument would carry more weight if it was an actual argument, instead of something else.

@Charlie Marlow said:

I would much rather have single exit point from a method than to deal with gotcha return statements burried in nested loops as in the case of the code I posted.

It depends. In your case, you're probably right. But the discussion single/multiple expit points has been done.

TwelveBaud1

@Juifang said:

AFAIK, all "" string literals share the same copy, at least in
mono.That means it should be exactly the same as string.Empty. Don't
know if it's different for MS.NET.

Same in MS.NET.@Weng said:

String.empty, as far as I can tell, is an artifact of the API having to support VB. I have never seen a VB reference, or a VB coder use anything other than String.empty, which leads me to believe that "" is somehow invalid syntax.

Nope, and nope. Two double-quotes right next to each other is the empty string, unless it's within another string, in which case it becomes a single double quote.

morbiuswilters

@dhromed said:

It depends. In your case, you're probably right. But the discussion single/multiple expit points has been done.

Goddammit, starting array indexes at 0 makes no fucking sense, man!!

Oh, wait...

bstorer

@morbiuswilters said:

@dhromed said:
It depends. In your case, you're probably right. But the discussion single/multiple expit points has been done.
Goddammit, starting array indexes at 0 makes no fucking sense, man!!

Oh, wait...

That's because VB is TRWTF!

bjolling

@bstorer said:

@morbiuswilters said:
@dhromed said:
It depends. In your case, you're probably right. But the discussion single/multiple expit points has been done.
Goddammit, starting array indexes at 0 makes no fucking sense, man!!

Oh, wait...
That's because VB is TRWTF!

It's all the fault of M$, the convicted monopolist!

But it runs better on a mac