Is there unreachable code in this snippet? I don't think so, but Resharper is telling me otherwise.

Question

I have the following method I came across in a code review. Inside the loop Resharper is telling me that if (narrativefound == false) is incorrect becuase narrativeFound is always true. I don't think this is the case, because in order to set narrativeFound to true it has to pass the conditional string compare first, so how can it always be true? Am I missing something? Is this a bug in Resharper or in our code?

public Chassis GetChassisForElcomp(SPPA.Domain.ChassisData.Chassis asMaintained, SPPA.Domain.ChassisData.Chassis newChassis)
{
    Chassis c = asMaintained;
    List<Narrative> newNarrativeList = new List<Narrative>();

    foreach (Narrative newNarrative in newChassis.Narratives)
    {
         bool narrativefound = false; 

         foreach (Narrative orig in asMaintained.Narratives)
         {
                if (string.Compare(orig.PCode, newNarrative.PCode) ==0 )
                {
                          narrativefound = true;
                          if (newNarrative.NarrativeValue.Trim().Length != 0)
                          {
                             orig.NarrativeValue = newNarrative.NarrativeValue;
                             newNarrativeList.Add(orig);                            
                          }
                          break;
                }
                if (narrativefound == false)
                {
                     newNarrativeList.Add(newNarrative); 
                }
         }
    }

    c.SalesCodes = newChassis.SalesCodes;
    c.Narratives = newNarrativeList;
    return c; 
}

Answer 1

The variable narrativefound will never be true when control reaches that statement:

narrativefound = true;
// ...
break;  // This causes control to break out of the loop.

I think Resharper is trying to tell you that the condition narrativefound == false will always be true.

Answer 2

R# is correct because if you turn narrativefound to true you are breaking out of the foreach right after you set it.

Answer 3

You don't need the narrativeFound variable at all. In the scope where you set it true, you break from the foreach loop. If you don't set it to true, you don't break, and you add the newNarrative to the newNarrativeList.

So, this could be rewritten as

foreach (Narrative newNarrative in newChassis.Narratives)
{
     foreach (Narrative orig in asMaintained.Narratives)
     {
            if (string.Compare(orig.PCode, newNarrative.PCode) == 0)
            {
                      if (newNarrative.NarrativeValue.Trim().Length != 0)
                      {
                         orig.NarrativeValue = newNarrative.NarrativeValue;
                         newNarrativeList.Add(orig);                            
                      }
                      break;
            }

            newNarrativeList.Add(newNarrative);                 
     }
}

Answer 4

It's a bug in your code.

foreach (Narrative newNarrative in newChassis.Narratives) 
{ 
     bool narrativefound = false;  

     foreach (Narrative orig in asMaintained.Narratives) 
     { 
            if (string.Compare(orig.PCode, newNarrative.PCode) ==0 ) 
            { 
                      narrativefound = true; 
                      if (newNarrative.NarrativeValue.Trim().Length != 0) 
                      { 
                         orig.NarrativeValue = newNarrative.NarrativeValue; 
                         newNarrativeList.Add(orig);                             
                      } 
// narrativeFound == true, but now we exit the for loop
                      break; 
            } 
// narrativeFound is always false here.  The test is redundant
            if (narrativefound == false) 
            { 
                 newNarrativeList.Add(newNarrative);  
            } 
     } 
} 

Answer 5

I believe it is telling you that becuase IF narriativefound is set to true then the for loop is exited (break;). So if the if (narriativefound == false) is evaluated it will always have a value of false.