Skip to main content
replaced http://codereview.stackexchange.com/ with https://codereview.stackexchange.com/
Source Link

As has been said by @jrh@jrh, having your PlayersSocket returned by a public getter is a flag that your classes encapsulation has a hole in it and might not make sense. Once the socket has been passed into the constructor for PlayerConnection, the instance should take complete responsibility for the Socket. If it allows access to the raw socket, then it can't maintain that level of control. As it stands, there's nothing stopping a client class calling:

As has been pointed out by @jrh@jrh, ListenForPackets is marked as private and doesn't appear to be called from within the class. The method itself is using packetsActive as a flag to indicate if the method should actually do anything or not. This suggests it might be expecting to be called when a BeginReceive has already been called. This suggests that the socket might not be being managed as well as I would expect, however without the calling code it's hard to say.

As has been said by @jrh, having your PlayersSocket returned by a public getter is a flag that your classes encapsulation has a hole in it and might not make sense. Once the socket has been passed into the constructor for PlayerConnection, the instance should take complete responsibility for the Socket. If it allows access to the raw socket, then it can't maintain that level of control. As it stands, there's nothing stopping a client class calling:

As has been pointed out by @jrh, ListenForPackets is marked as private and doesn't appear to be called from within the class. The method itself is using packetsActive as a flag to indicate if the method should actually do anything or not. This suggests it might be expecting to be called when a BeginReceive has already been called. This suggests that the socket might not be being managed as well as I would expect, however without the calling code it's hard to say.

As has been said by @jrh, having your PlayersSocket returned by a public getter is a flag that your classes encapsulation has a hole in it and might not make sense. Once the socket has been passed into the constructor for PlayerConnection, the instance should take complete responsibility for the Socket. If it allows access to the raw socket, then it can't maintain that level of control. As it stands, there's nothing stopping a client class calling:

As has been pointed out by @jrh, ListenForPackets is marked as private and doesn't appear to be called from within the class. The method itself is using packetsActive as a flag to indicate if the method should actually do anything or not. This suggests it might be expecting to be called when a BeginReceive has already been called. This suggests that the socket might not be being managed as well as I would expect, however without the calling code it's hard to say.

Source Link
forsvarir
  • 11.8k
  • 7
  • 39
  • 72

Beware the leaky abstraction

As has been said by @jrh, having your PlayersSocket returned by a public getter is a flag that your classes encapsulation has a hole in it and might not make sense. Once the socket has been passed into the constructor for PlayerConnection, the instance should take complete responsibility for the Socket. If it allows access to the raw socket, then it can't maintain that level of control. As it stands, there's nothing stopping a client class calling:

playerConnection.PlayersSocket.Close();

Unused Usings

As far as I can tell, you're not using anything from these namespaces:

using System.Collections.Generic;
using System.Linq;
using System.Threading.Tasks;

If you're not using them, remove them. Anything that doesn't need to be there just adds clutter. It's the same for commented out code. If you don't need it, get rid of it. When you see a line like:

if (bytesReceived == 0 /*|| bytesReceived != typeof(int)*/)

It just makes you wonder why it's been commented out. Should it really be uncommented, is the comment to get it to go down a specific path. If the code had just been removed, it would be easier to read.

DecodeInt32 - exceptions are your friend

This makes no sense to me:

if ((((toDecode[0] | toDecode[1]) | toDecode[2]) | toDecode[3]) < 0)
    return -1;

byte is an unsigned type, it's never going to be < 0. And if you somehow managed to get it to trigger this criteria, would you really want the method to return -1? -1 seems like a fairly expected value when decoding an int32, if you decide that you've got unexpected data and you don't know what to do with it, throw an exception, don't just fake a return value. Faking a value is likely to lead to hidden bugs in the future. Better to throw an InvalidOperationException or ArgumentException and know you have a problem that you need to deal with.

Events and Delegates

Having a separation in concerns between the Player and PlayerConnection makes sense in a lot of scenarios, not least it allows you to maintain the Player state if a player loses a connection and then reconnects through a different Socket. However, for this to work there has to be some way for the Player and the Connection to communicate. One approach might be for the player to register a delegate / event with the connection so that it gets a notification on packet received (or a higher level abstraction). There's no obvious way in your existing code for data to get from the Connection to the Player. This suggests that the abstractions in your code don't entirely make sense.

ListenForPackets

As has been pointed out by @jrh, ListenForPackets is marked as private and doesn't appear to be called from within the class. The method itself is using packetsActive as a flag to indicate if the method should actually do anything or not. This suggests it might be expecting to be called when a BeginReceive has already been called. This suggests that the socket might not be being managed as well as I would expect, however without the calling code it's hard to say.

CloseConnection

This method doesn't really do anything other than setting isPlayerDisconnecting=true. Checking if it's already true beforehand seems somewhat superfluous, you might as well just always set it:

public void CloseConnection()
{
    isPlayerDisconnecting = true;
}

For what it's worth, I'm not entirely sure CloseConnection is the right name for something that doesn't actually close the connection. The flag is used to prevent more received data from being processed, not to trigger a socket closure.

When you're done, you're done

OnPacketsReceived is used to process received information. It wraps the processing in a try/catch/finally block. The finally block triggers another read. This makes sense in scenarios where you're keeping the socket open and you want to instruct it to keep receiving data over and over again. However, it doesn't make sense after the class has been instructed to close the connection. This:

try
{
    int bytesReceived;

    if (isPlayerDisconnecting)
        return;

Should probably be changed to:

if (isPlayerDisconnecting)
    return;
try
{
    int bytesReceived;

As it stands, the method is using a isPlayerDisconnecting to decide that it should ignore any data that has arrived, but then goes into the finally block and kicks off another read (which again will be ignored). This seems wrong.

Packet Handling

It looks like you're using the first byte received as a packet type indicator. Depending upon the number of different packet types, you might want to break out the extraction. I would also consider breaking out the logic for constructing the packet content from the connection class. This will separate the responsibility for managing the buffer from the responsibility of managing the connection. This should make both classes easier to follow and test.