[C#] Bot stuck in loop...

[Insolence]

Code Select Expand

public class Message
{
//MSG_ID LAST_MESSAGE;


const string MSG_ID_GAME = "0000";
const string MSG_ID_CHAT = "0010";
const string MSG_ID_USER            = "1001";
const string MSG_ID_JOIN                = "1002";
const string MSG_ID_LEAVE                = "1003";
const string MSG_ID_WHISPER              = "1004";
const string MSG_ID_TALK                = "1005";
const string MSG_ID_BROADCAST            = "1006";
const string MSG_ID_CHANNEL              = "1007";
const string MSG_ID_USERFLAGS            = "1009";
const string MSG_ID_WHISPERSENT          = "1010";
const string MSG_ID_CHANNELFULL          = "1013";
const string MSG_ID_CHANNELDOESNOTEXIST = "1014";
const string MSG_ID_CHANNELRESTRICTED    = "1015";
const string MSG_ID_INFO                = "1018";
const string MSG_ID_ERROR                = "1019";
const string MSG_ID_EMOTE                = "1023";
const string MSG_ID_NAME = "2010";

public string Parse ( string msg )
{
//MessageBox.Show ( msg );
string returnString = "";

foreach ( string Line in msg.Split ( new char [] { '\n' } ) )
{


                                // Doesn't even make sense, it just gets stuck right here, I commented this part out and it displays the data back correctly




if ( Char.IsDigit(Line[0]) )
{
string [] Word = Line.Split ( new char [] { ' ' } );

switch ( Word[0] )
{
case MSG_ID_CHANNEL:
return "Joining " + Word[2];
}

MessageBox.Show ( Line );
}






}

return returnString;
}
}

[MyndFyre]

A couple things I would suggest -- they aren't necessarily do-or-die suggestions, but they'll probably help with debugging and stepping through.

1.) Don't create variables except for simple counting variables in loops.  This part:

Code Select Expand

string Line in msg.Split ( new char [] { '\n' } )
should be split up before your foreach:

Code Select Expand

string msgParts = msg.Split(new char[] { '\n' });
foreach (string Line in msgParts) { ... }


2.) To further your debugging aid, use a simple for loop rather than a foreach loop.  With a simple for loop, you can see what iteration you are on and more easily see during debugging what the terminating condition is.

Code Select Expand


int partsLength = msgParts.Length;
for (int i = 0; i < partsLength; i++)
{
...
}


Last, as a general courtesy to other people on the forum, 1.) post this in an on-topic forum: while this has to do with your bot, it's a general or .NET programming question, not a bot programming question; and 2.) Actually write a message and don't just throw code at us.  I was ready to reply in disgust until I came across your comment.  Comments and code all appear the same color.

[Insolence]

Oh sorry, it was 4 in the morning after literally 8 hours of trying to fix with a few games of SC inbetween

I'll be sure to post in an on-topic forum next time.  Thanks for the tips, also, but do you have any idea why it's getting stuck there?  It just stands still, it doesn't hit the end of the function.

[MyndFyre]

Oh sorry, it was 4 in the morning after literally 8 hours of trying to fix with a few games of SC inbetween

I'll be sure to post in an on-topic forum next time.  Thanks for the tips, also, but do you have any idea why it's getting stuck there?  It just stands still, it doesn't hit the end of the function.

Step through in the debugger and watch the value of i if you change the foreach loop to the for loop I suggested.  It may never be reaching a terminating condition.

[shout]

It may never be reaching a terminating condition.

In a foreach loop, that would mean that the Line variable would be an infititly large string. It should take almost no time to interate through that, even if it is hundreds of thousands of characters long.

@Insolence:

Bad boy! Are you using a string as a buffer? This section of code makes me think so:

Code Select Expand


case MSG_ID_CHANNEL:
return "Joining " + Word[2];


Also, the

Code Select Expand


string msgParts = msg.Split(new char[] { '\n' });

could be written much simpler as:

Code Select Expand


string msgParts = msg.Split(0);

Or the 0 can be 0x00, '\n', or even Environment.Newline.

Edit: Where is the break in that switch statement? I did not know that could even compile.

[MyndFyre]

It may never be reaching a terminating condition.

In a foreach loop, that would mean that the Line variable would be an infititly large string. It should take almost no time to interate through that, even if it is hundreds of thousands of characters long.

I agree, but at the same time, I've got no idea what it could be.  The code looks fine.  I say, change it up a little and step through -- maybe we'll get to see what's up with the little bugger.

@Insolence:

Bad boy! Are you using a string as a buffer? This section of code makes me think so:

Code Select Expand


case MSG_ID_CHANNEL:
return "Joining " + Word[2];


I don't know why you think he's using a string as a buffer, but he *is* using a CHAT protocol client.  There's no binary about it -- all the data is in string form.  You can't use a string as a buffer in C# with sockets.

Also, the

Code Select Expand


string msgParts = msg.Split(new char[] { '\n' });

could be written much simpler as:

Code Select Expand


string msgParts = msg.Split(0);

Or the 0 can be 0x00, '\n', or even Environment.Newline.

Except that's not the same.  The escape sequence for 0x00 is '\0', not '\n'.  And you can't use Environment.NewLine there, because the Split() method takes a char[] or a params char[], and Environment.NewLine is a string (CRLF).  '\n' is a newline, not NULL.

Edit: Where is the break in that switch statement? I did not know that could even compile.

The C# compiler requires that you have a terminating condition in switch cases, not a break statement -- to prevent fall-through.  If he was to code:

Code Select Expand


switch ( Word[0] )
{
case MSG_ID_CHANNEL:
return "Joining " + Word[2];
break;
}

The C# compiler would highlight the break (well, in the IDE) and emit the warning "Unreachable code detected."  This requirement of the compiler is designed to prevent the following type of code from being compiled:

Code Select Expand


switch (operator)
{
case '+':
value = a + b;
case '-':
value = a - b;
break;
}

return value;

Note that the '+' case falls-through to the '-' case.  If the compiler didn't catch this error, it would cause a runtime error that might otherwise be difficult to pin down.  However, most branch instructions are legal to get out of the switch case -- break, return, continue when the switch statement is nested inside a loop, and I believe even goto is legal.

[Insolence]

Man, I love this forum.  MyndFyre, you're especially insightful. 

However, I've never used a debugger, but I'll try the For loop thing tomorrow.  Having friends over today.

[Kp]

This requirement of the compiler is designed to prevent the following type of code from being compiled:

Code Select Expand

switch (operator)
{
case '+':
value = a + b;
case '-':
value = a - b;
break;
}

return value;Note that the '+' case falls-through to the '-' case.  If the compiler didn't catch this error, it would cause a runtime error that might otherwise be difficult to pin down.

From your comments, am I correct in concluding that the quoted piece of code is rejected by the C# compiler, despite that fallthrough is a classic and extremely useful feature of C?  I've seen (and written!) many programs which exploit fallthrough to avoid code duplication in switch statements.  Fallthrough is even used in Linux kernel code.

[K]

I could have sworn fallthrough was allowed, but it appears you're right:

Code Select Expand


:~$ cat test.cs
using System;

namespace MyApp
{
   public class TestApp
   {
      public static void Main(string[] args)
      {
         switch(args[1])
         {
         case "hello":
            Console.WriteLine("Why hello!");
         case "hi":
            Console.WriteLine("How are you doing?");
            break;
         case "bye":
            break;
         }
      }
   };
};
:~$ mcs test.cs
test.cs(9) error CS0163: Control cannot fall through from one case label to another
Compilation failed: 1 error(s), 0 warnings


However, it appears fall through is allowed if there is no specific code for each case:

Code Select Expand


// allowed
switch(foo)
{
   case a:
   case b:
      break;
}


[MyndFyre]

From your comments, am I correct in concluding that the quoted piece of code is rejected by the C# compiler, despite that fallthrough is a classic and extremely useful feature of C?  I've seen (and written!) many programs which exploit fallthrough to avoid code duplication in switch statements.  Fallthrough is even used in Linux kernel code.

Correct.  Code duplication could be avoided via refactoring, a somewhat more safe procedure.

However, it appears fall through is allowed if there is no specific code for each case:

Again, correct.  I thought I'd mentioned that; I guess not.

[shout]

Sorry, just worked a 12 hour shift and I was a little confused

[Kp]

Correct.  Code duplication could be avoided via refactoring, a somewhat more safe procedure.

I'm not familiar with the term "refactoring."  How would you refactor the following code (taken from the Linux kernel)?

Code Select Expand

switch (how)
{
case 1:

if (test_bit(SOCK_ASYNC_WAITDATA, &sock->flags))
break;
goto call_kill;
case 2:
if (!test_and_clear_bit(SOCK_ASYNC_NOSPACE, &sock->flags))
break;
/* fall through */
case 0:
call_kill:
__kill_fasync(sock->fasync_list, SIGIO, band);
break;
case 3:
__kill_fasync(sock->fasync_list, SIGURG, band);
}In particular, I'm hoping to see a solution which does not increase the number of function calls emitted.  Also, are you ever going to rebut my example of what a real shell ought to do?   If cmd can even come close to the power of tcsh, I'd definitely like to hear about it.

[Insolence]

By the way, for future reference the problem was Line[0] being out of bounds, and the error occuring in a seperate thread.  A simple try/catch found it out for me