Howdy, Stranger!

It looks like you're new here. If you want to get involved, click one of these buttons!

Categories

Is it possable to inprove my code?

DevelopmentDevelopment Member Posts: 222
can anyone please inprove move code make it more stable?

[code]
procedure TfrmMain.Sock1Read(Sender: TObject; Socket: TCustomWinSocket);
var
After5Char, First5Char, First4Char, FullString : String;
Buffer : Array[0..255] of char;
begin
case ClientStatus of
csIdle:
begin
Socket.ReceiveBuf(Buffer,5);
First5Char := copy(buffer,1,5);
After5Char := Copy(FullString,6,Length(FullString));

if First5Char = 'CCON!' then // get connection
ClientStatus := csOnlineUsers
else

if First5char = 'USER!' then // user information ( Name )
ClientStatus := csUserText
else

if First5char = 'MSG!' then // send message to clients
ClientStatus := csMsg
else

if First5Char = '_MIC!' then
ClientStatus:= csTalking // You could add a Talking status too
else

if First5Char = 'TALK!'then // free up mic when no one is talking
ClientStatus:= csFinishedTalking
else

ShowMessage('full string = '+FullString+' first4char = '+First4Char);
if First4Char = 'URL!' then // send URL to users
begin
ShowMessage('you should see a site now.'); // inform yourself that it should be working
ClientStatus := csURL; // show the page
end;
end;

csURL: // send url to clients
begin
FullString := Sock1.Socket.ReceiveText; // revieve the test from the server
ShowMessage('Full string = '+FullString); // just for debugging purposes, show it
PlusHTML.Homepage := FullString; // set the homepage
PlusHTML.GoHomepage; // go to the homepage
PageControl1.ActivePageIndex := 1; // set the active page
ClientStatus := csIdle;
end;

csTalking: // user on mic talking
begin
SpeedButton1.Enabled:= False;
Name := Sock1.Socket.ReceiveText;
Panel9.Caption := Name;
ClientStatus := csIdle;
end;

csFinishedTalking: // free mic
Begin
SpeedButton1.Enabled:= True;
ClientStatus := csIdle;
end;

csOnlineUsers: // users online
begin
FullString := Socket.ReceiveText;
OnlineUserListBox.Items.Text := fullstring;
// PlaySound('newFriend.wav',0,SND_FILENAME);
ClientStatus := csIdle;
end;

csUserText: // send data to all users
begin
Fullstring := Socket.ReceiveText;
txtMain.Lines.Add(FullString);
ClientStatus := csIdle;
end;

csMsg: // system message
begin
FullString := Socket.ReceiveText;
txtMain.Lines.Add(FullString);
Application.MessageBox(PChar(FullString), 'System Information', mb_Ok);
ClientStatus := csIdle;
end;
end;
end;
[/code]
Slewis

Comments

  • jammojammo Member Posts: 12
    : can anyone please inprove move code make it more stable?

    Hi, the main problem might be where you have this line:

    [code]
    After5Char := Copy(FullString,6,Length(FullString));
    [/code]

    Try changing it to:

    [code]
    After5Char := Copy(FullString,6,Length(FullString)-6);
    [/code]

    as you may be ovverrunning the buffer. The last parameter is a count, not a position. So if you have a string of length 10, and you copy from position 4 to the length of the string, it will try and access up to string parts 14. Which is overrunning it.

    if that doesn't work, try this code out. I've edited it a bit:

    [code]
    procedure TfrmMain.Sock1Read(Sender: TObject; Socket: TCustomWinSocket);
    var
    After5Char, First5Char, First4Char, FullString : String;
    Buffer : Array[0..255] of char;
    begin

    case ClientStatus of
    csIdle:
    begin
    Socket.ReceiveBuf(Buffer,1024);
    FullString := copy(buffer,1,1024);
    First5Char := Copy(FullString,1,5);
    After5Char := Copy(FullString,6,Length(FullString)-6);

    //*** First4Char not defined, so added
    First4Char:= MidStr(First5Char,1,4);

    if First5Char = 'CCON!' then // get connection
    ClientStatus := csOnlineUsers
    else if First5char = 'USER!' then // user information ( Name )
    ClientStatus := csUserText
    else if First5Char = '_MIC!' then
    ClientStatus:= csTalking // You could add a Talking status too
    else if First5Char = 'TALK!'then // free up mic when no one is talking
    ClientStatus:= csFinishedTalking
    else
    ShowMessage('full string = '+FullString+' first4char = '+First4Char);

    //*** Fullstring has not been defined or set yet!

    If First4char = 'MSG!' then // send message to clients
    //*** Condition is only 4 chars long. Changed to First4Char
    ClientStatus := csMsg
    else if First4Char = 'URL!' then // send URL to users
    begin
    ShowMessage('you should see a site now.'); // inform yourself that it should be working
    ClientStatus := csURL; // show the page
    end; //if
    end; //case csIDLE

    csURL: // send url to clients
    begin
    FullString := Sock1.Socket.ReceiveText; // revieve the test from the server
    ShowMessage('Full string = '+FullString); // just for debugging purposes, show it
    PlusHTML.Homepage := FullString; // set the homepage
    PlusHTML.GoHomepage; // go to the homepage
    PageControl1.ActivePageIndex := 1; // set the active page
    ClientStatus := csIdle;
    end;

    csTalking: // user on mic talking
    begin
    SpeedButton1.Enabled:= False;
    Name := Sock1.Socket.ReceiveText;
    Panel9.Caption := Name;
    ClientStatus := csIdle;
    end;

    csFinishedTalking: // free mic
    Begin
    SpeedButton1.Enabled:= True;
    ClientStatus := csIdle;
    end;

    csOnlineUsers: // users online
    begin
    FullString := Socket.ReceiveText;
    OnlineUserListBox.Items.Text := fullstring;
    // PlaySound('newFriend.wav',0,SND_FILENAME);
    ClientStatus := csIdle;
    end;

    csUserText: // send data to all users
    begin
    Fullstring := Socket.ReceiveText;
    txtMain.Lines.Add(FullString);
    ClientStatus := csIdle;
    end;

    csMsg: // system message
    begin
    FullString := Socket.ReceiveText;
    txtMain.Lines.Add(FullString);
    Application.MessageBox(PChar(FullString), 'System Information', mb_Ok);
    ClientStatus := csIdle;
    end;

    end; //case

    end;
    [/code]

  • KoppisKoppis Member Posts: 68
    : Hi, the main problem might be where you have this line:
    :
    : [code]
    : After5Char := Copy(FullString,6,Length(FullString));
    : [/code]
    :
    : Try changing it to:
    :
    : [code]
    : After5Char := Copy(FullString,6,Length(FullString)-6);
    : [/code]

    Shouldn't it be

    [code]
    After5Char := Copy(FullString,6,Length(FullString)-5);
    [/code]

    Think about FullString length as 6, then it would not take the character 6 because 6-6=0 and so Count is zero.
  • zibadianzibadian Member Posts: 6,349
    [b][red]This message was edited by zibadian at 2004-5-18 3:6:37[/red][/b][hr]
    : : Hi, the main problem might be where you have this line:
    : :
    : : [code]
    : : After5Char := Copy(FullString,6,Length(FullString));
    : : [/code]
    : :
    : : Try changing it to:
    : :
    : : [code]
    : : After5Char := Copy(FullString,6,Length(FullString)-6);
    : : [/code]
    :
    : Shouldn't it be
    :
    : [code]
    : After5Char := Copy(FullString,6,Length(FullString)-5);
    : [/code]
    :
    : Think about FullString length as 6, then it would not take the character 6 because 6-6=0 and so Count is zero.
    :
    That is not a line, which makes the code unstable, since the help files clearly state:
    [code]
    If Count specifies more characters or array elements than are available, only the characters or elements from S[Index] to the end of S are returned.
    [/code]
    So the number given as Count can savely be Length(FullString).

    The a few of the problems are these:
    - FullString isn't filled
    - Only 5 bytes are read from socket stream, while the program expects more
    - Several of conditions in the the first if-then-else statement have ony 4 characters, where 5 are expected
    - There is also a First4Char, which isn't set anywhere

Sign In or Register to comment.