Search code examples
c++clabelgoto

How can I refactor this C++ to remove the labels/gotos?


My problem is not really a problem, but I want to make this code look a bit more elegant than it currently is. It has spaghetti code.

Here is a little example of what I got.

 int func(...) {
  if ( ... )
  {
    v6 = ESI_1C;
    EBP = 1;
    v5 = 0;
    v21 = 0;
    v19 = 0;
    v25 = ESI_1C;
    if ( packetSize > 1 )
    {
      v26 = (unsigned __int8)initialCryptAnswer;
      v10 = v8 - v6;
      v11 = v6 + 4;
      for ( i = v8 - v6; ; v10 = i )
      {
        v12 = *(DWORD *)(v10 + v11);
        v21 += v12;
        cryptAnswer = (unsigned __int8)cryptTable[(2 * (unsigned __int8)v26)+1];
        v13 = EBP & 3;
        if ( !(EBP & 3) )
          break;
        if ( v13 == 1 ) {
          v16 = cryptAnswer >> 1;
          *(DWORD *)v11 = v12 - v16;
        } else if ( v13 == 2 ) {
          *(DWORD *)v11 = v12 + 2 * cryptAnswer;
        } else if ( v13 == 3 ) {
          v16 = cryptAnswer >> 2;
          *(DWORD *)v11 = v12 - v16;
        }
LABEL_19:
        v17 = *(DWORD *)v11 + v19;
        ++EBP;
        v11 += 4;
        v19 = v17;
        ++v26;
        if ( EBP >= packetSize )
        {
          ESI = v24;
          v5 = v17;
          v6 = v25;
          goto LABEL_22; //kinda like break it's much more because it's instead of a for loop too.
        }
      }
      *(DWORD *)v11 = v12 + 4 * cryptAnswer;
      goto LABEL_19; //this is a looper
    }
LABEL_22:
    result = 1;
  } else {
    result = 0;
  }
  return result;
}

What can I do to get rid of the labels without messing up the code flow as it's very important part of an encryption function?


Solution

  • Second answer, as the question changed a lot.

    The original code looks like, after removing all redundant stuff:

    for (... ; ; ...)
    {
      ...
      if (...)
      {
        break;
      }
      ABC
    LABEL_19:
      DEF
    }
    XYZ;
    goto LABEL_19;
    

    This is exactly the same as:

    for (... ; ; ...)
    {
      ...
      if (...)
      {
        XYZ;
      }
      else
      {
        ABC
      }
    
      DEF
    }
    

    Once you have done this rewrite, you could replace the goto LABEL_22 with a simple break.

    EDIT: Or more explicitly:

    for (... ; ; ...)
    {
      ...
      if (! (ESP & 3) )
      {
        // This line used to be located after the loop.
        *(DWORD *)v11 = v12 + 4*cryptAnswer;  // 111
      }
      else
      {
        if (v13 == 1)
        {
          ...
        }
        else if (v13 == 2)
        {
          ...
        }
        else if (v13 == 3)
        {
          ...
        }
      }
    
      // This is where LABEL_19 used to be.
    
      v17 = ...      // 222
    }
    

    The rule of thumb when you rewrite code like this is that the code should perform the same actions in the same order as before. You simply can't move code around blindly, and to do this you must understand the flow of the code. In this case, when the first if is taken, the line I've marked with // 111 is first executed, then the line // 222, nothing else.