A Message to all developers

01/03/2015 13:56 Ultimation#1
I have been taking some time and going over peoples sources.. and peoples modifications..

I would like to point out a few things.

OOP = Object oriented programming..

Quote from wikipedia

Quote:
Object-oriented programming (OOP) is a programming paradigm based on the concept of "objects", which are data structures that contain data, in the form of fields, often known as attributes; and code, in the form of procedures, often known as methods. A distinguishing feature of objects is that an object's procedures can access and often modify the data fields of the object with which they are associated (objects have a notion of "this"). In object-oriented programming, computer programs are designed by making them out of objects that interact with one another.[1][2] There is significant diversity in object-oriented programming, but most popular languages are class-based, meaning that objects are instances of classes, which typically also determines their type.
This means 2 very big things you guys seem to get the grasp.

use constructors... Why do so many sources use functions as constructors...

Example..

Code:
var actionpckt = ActionPacket.Create(); ??????? or
var actionpckt = new ActionPacket().Create(); ??????????
this is just plain ignorance. Get out of your c++/delphi ways

Code:
var actionpckt = new ActionPacket(); //Clean and simple
Second thing.

STOP putting all your fucking handlers in 1 class in 1 file....

You have ways todo this.. 1 class handler per packet..

OR

Partial Classes! Take your pick!

Example

Code:
Action Handler.cs
public partial class PacketHandler
{
public void ProcessAction(params)
{

}
}

Item Handler.cs
public partial class PacketHandler
{
public void ProcessItem(params)
{

}
}

Third Thing

If else statements...

Dont use them if you dont need to.

doing

Code:
If (packet.type == xxxx)
{
}
else
if (packet.type == xxxx)
{
}
is just pure lazy and inefficient.. Use the CASE statement

Code:
switch (packet.type)
{
case xxxx:
{
PacketHandler.ProcessAction(packet);
}
case xxxx:
{
PacketHandler.ProcessItemUsage(packet);
}
}
Finally...

USE SEARCH!


STOP BEING SO LAZY PEOPLE AND CODERS MAY HELP YOU! AND THIS COMMUNITY MAY JUST MAY GET BACK ON TRACK!
01/03/2015 14:07 .Ocularis#2
I see what you're saying about using constructors, however.. some packet archetypes have different fields, like general action and item action. Using a method after the constructor to construct the archetype is a bad practice? If a constructor were made for each individual packet archetype some parameters would make several constructors ambiguous even though each constructor would write those values to different fields..

Edit:
My only other thought to avoid using methods to construct archetypes is to use an abstract structure and inheritance for the individual types, serializing them as the abstract structure in the constructor of the packet.
01/03/2015 14:11 Ultimation#3
Let me paste my packets for some types.. They may be incomplete but the concept is there

Code:
 
public class MessagePacket:IPacket
    {
        public string From="SYSTEM";
        public string To="ALLUSERS";
        public string Message="";
        public ChatID ChatID=ChatID.Talk;
        public int Avatar = 0;
        public int Color = 0xFFFFFF;
        public MessagePacket(string from, string to, string message,ChatID chatid=ChatID.Talk)
        {
            From = from;
            To = to;
            Message = message;
            ChatID = chatid;
        }
        public MessagePacket(string To, string Message, ChatID chatid = ChatID.Talk)
        {
            this.To = To;
            this.Message = Message;
            ChatID = chatid;
        }
        public MessagePacket()
        {
            
        }
        public MessagePacket(string Message, ChatID chatid = ChatID.Talk)
        {
            this.Message = Message;
            ChatID = chatid;
        }

        public int PacketType { get { return 0x3EC; }}
        public byte[] Build()
        {
            using (var wtr = new PacketWriter(0x3EC))
            {
                wtr.Write(Color);
                wtr.Write((int)ChatID);
                wtr.Write(0);
                wtr.Write(0);
                wtr.Write(Avatar);
                wtr.WriteStringList(From,To,string.Empty,Message);
                return wtr.ToArray();
            }
        }

        public void Read(byte[] data)
        {
            using (var br = new BinaryReader(new MemoryStream(data)))
            {
                br.ReadInt32();
                Color = br.ReadInt32();
                ChatID = (ChatID) br.ReadInt32();
                br.ReadInt32();
                br.ReadInt32();
                Avatar = br.ReadInt32();
                var dat= br.ReadStringList();
                From = dat[0];
                To = dat[1];
                Message = dat[3];
            }
        }
Code:
public class ItemUsagePacket:IPacket
    {
        public int PacketType { get { return 0x3F1; }}
        public int UID;
        public int dwParam1;
        public ItemUsuageID ID;
        public int TimeStamp;
        public int dwParam2;
        public int dwParam3;
        public int[] ItemIDS = new int[16];

        public ItemUsagePacket() {  }

        public ItemUsagePacket(int PlayerID)
        {
           
            TimeStamp = Environment.TickCount ^ PlayerID;
            UID = PlayerID;
        }
        public byte[] Build()
        {
            using (var wtr = new PacketWriter(0x3F1))
            {

                wtr.Write((int)UID);
                wtr.Write((int)dwParam1);
                wtr.Write((int)ID);
                wtr.Write((int)TimeStamp);
                wtr.Write((int)dwParam2);
                wtr.Write((int)dwParam3);
                wtr.Write((int)0);
                foreach (int id in ItemIDS)
                    wtr.Write((uint)id);
                return wtr.ToArray();
            }
        }

        public void Read(byte[] data)
        {
            using (var rdr = new BinaryReader(new MemoryStream(data)))
            {
                rdr.ReadInt32();
                UID = rdr.ReadInt32();
                dwParam1 = rdr.ReadInt32();
                ID = (ItemUsuageID)rdr.ReadInt32();
                TimeStamp = rdr.ReadInt32();
                dwParam2 = rdr.ReadInt32();
                dwParam3 = rdr.ReadInt32();
            }
        }
Quote:
Originally Posted by .Ocularis View Post
I see what you're saying about using constructors, however.. some packet archetypes have different fields, like general action and item action. Using a method after the constructor to construct the archetype is a bad practice? If a constructor were made for each individual packet archetype some parameters would make several constructors ambiguous even though each constructor would write those values to different fields..
Regarding bad practise.. there should never be a need to use an Ambiguous function as a constructor for a class
01/03/2015 14:22 .Ocularis#4
Aha.
Now I see.. my style and perfectionism doesn't like:
Code:
new Action(){uid = 1, type = 2, argument = 3};
That's why I'd use methods... but you're right, using methods is totally excessive and retarded.
Will work on it.
01/03/2015 14:29 Ultimation#5
Quote:
Originally Posted by .Ocularis View Post
Aha.
Now I see.. my style and perfectionism doesn't like:
Code:
new Action(){uid = 1, type = 2, argument = 3};
That's why I'd use methods... but you're right, using methods is totally excessive and retarded.
Will work on it.
Happy to help :)
01/03/2015 16:04 Best Coder 2014#6
Quote:
Originally Posted by Ultimation View Post
Let me paste my packets for some types.. They may be incomplete but the concept is there



Regarding bad practise.. there should never be a need to use an Ambiguous function as a constructor for a class
You talk about OOP, but you're breaking one of the fundamental priciples of OOP - encapsulation. Your beloved constructor is completely useless since you're publicly exposing all the internal fields of the class anyways.

These "functions as constructors" are actually an OOP concept as well. It's called factory methods. I'm not saying they're used properly in most CO pserver sources, but to say they're not OOP is wrong. An example of good use of a factory method can actually be found in TQ's CNetMsg class:


Not really related to OOP, but:
Speaking of Delphi/Pascal style, your [Only registered and activated users can see links. Click Here To Register...] is super frustrating. Sometimes you use PascalCase for parameter names, other times camelCase. What gives? At least be consistent. I know there's no official standard on this, but I don't think you'll ever find any serious people who'll tell you to use PascalCase for parameter names in C#.
01/03/2015 16:59 CptSky#7
Quote:
Originally Posted by Best Coder 2014 View Post
You talk about OOP, but you're breaking one of the fundamental priciples of OOP - encapsulation. Your beloved constructor is completely useless since you're publicly exposing all the internal fields of the class anyways.

These "functions as constructors" are actually an OOP concept as well. It's called factory methods. I'm not saying they're used properly in most CO pserver sources, but to say they're not OOP is wrong. An example of good use of a factory method can actually be found in TQ's CNetMsg class:

[...]

Not really related to OOP, but:
Speaking of Delphi/Pascal style, your [Only registered and activated users can see links. Click Here To Register...] is super frustrating. Sometimes you use PascalCase for parameter names, other times camelCase. What gives? At least be consistent. I know there's no official standard on this, but I don't think you'll ever find any serious people who'll tell you to use PascalCase for parameter names in C#.
Although he shouldn't expose all the fields of the class publicly (it should probably be a structure at this point), his constructor is not useless as some fields are read-only. Anyway, his point on constructors is not wrong. Most public sources don't respect such basic OOP concepts.

And, you should read again about the factory design pattern as it is not the pattern implemented in most public sources. Having a static method in each class to create the object is not a factory design pattern. The only time it could be right to do it, is if the constructor can fail, so you basically have also error checking on a building fonction, but with exceptions in C#, you shouldn't do it either. The factory design pattern is what you shown in the example from the TQ source. It allows to create an object without knowing exactly the return type.

Finally, you're totally right. His naming convention is not consistent at all. And I don't understand why hungarian notation is still used in strongly typed languages like C#.
01/03/2015 17:18 Best Coder 2014#8
Quote:
Originally Posted by CptSky View Post
Although he shouldn't expose all the fields of the class publicly (it should probably be a structure at this point), his constructor is not useless as some fields are read-only. Anyway, his point on constructors is not wrong. Most public sources don't respect such basic OOP concepts.

I don't see any read-only fields. I'm not disagreeing with you or Ultimation, I'm just pointing out some flaws in his example code.

Quote:
Originally Posted by CptSky View Post
And, you should read again about the factory design pattern as it is not the pattern implemented in most public sources. Having a static method in each class to create the object is not a factory design pattern. The only time it could be right to do it, is if the constructor can fail, so you basically have also error checking on a building fonction, but with exceptions in C#, you shouldn't do it either. The factory design pattern is what you shown in the example from the TQ source. It allows to create an object without knowing exactly the return type.
I know, that's why I said: "I'm not saying they're used properly in most CO pserver sources". My point was just that this concept of "functions as constructors" - factory methods - isn't necessarily a bad thing in OOP.
01/03/2015 17:48 CptSky#9
Quote:
Originally Posted by Best Coder 2014 View Post
[...]
I don't see any read-only fields. I'm not disagreeing with you or Ultimation, I'm just pointing out some flaws in his example code.
Was checking the ItemUsagePacket (but with a second thoughts, I think PacketType should be a constant, and shouldn't be considered as a read-only field).

Quote:
Originally Posted by Best Coder 2014 View Post
[...]I know, that's why I said: "I'm not saying they're used properly in most CO pserver sources". [...]
Must have skipped this sentence... :p I agree with you though.
01/03/2015 19:04 .John#10
To be honest, a switch is pretty shit as well.
It's far cleaner and better to split all your processing into classes / managers.
Ex:

Code:
public WarehouseManager : IEnumerable<Items>,  ...
{
       private Player _player;
       public WarehouseManager(Player player)
       {
             _player = player;
            _player.NetworkHandler[PacketType.Warehouse] = OnWarehousePacket;
        }


        private void OnWarehousePacket(Packet p)
        {


         }
}
Then you can easily do something like

Code:
foreach(var item in player.Warehouse) 
    //some code

foreach(var item in player.Inventory)
    //some code

or better yet

var questItem = player.Inventory.First(...)
var questItemCount = player.Inventory.Sum(x => x.TypeId == 1337 ? x.StackCount : 0 )

etc
Here is a bit more info that may help everyone:

The biggest thing about coding a game server is know how to properly split everything up so it reduces the amount of time you have to code.

Comments are optional. The purpose of a comment is to explain the idea / purpose of a function (or perhaps an in-depth analysis), not to tell a person "This function sets the stamina". Code should be self explanatory through design. Ex: Client.Inventory.Deposit(item). Pretty obvious what it does, what might not be so obvious is what happens when the item fails to deposit.

Also, LINQ is your biggest friend (if you're using C#). It's the different between 3 lines of code, or 30. Worry about performance, but don't make it your sole priority. We have pretty damn fast CPUs these days, and CO requires quite little processing (in fact, our server, COPS manages to use on average 2-5% CPU with 100+ players, with monster AI being active, events, etc)

Edit:

As for packet processing, here is the latest packet management I added to ConquerPS. An automatic structure parser via Reflect.Emit (meaning it's blazing fast after the first call, no performance issues). It basically packs/unpacks packets via structs/classes. The packet parser it's self is a reflection scanner which scans all classes at start-up for packet handlers ( a bit more complex, but that's the general idea)

Code:
   [PacketHandler(PacketType.Guild)]
        private void OnGuildRequestPacket(Packets.Guild data)
        {
            switch (data.Action)
            {
                case GuildAction.ApplyJoin:
                    {
                        var c = Game.Players[data.Data];
                        if (c != null)
                            Game.Guilds.Join(this, c);
                        break;
                    }
                case GuildAction.InviteJoin:
                    {
                        var c = Game.Players[data.Data];
                        if (c != null)
                            Game.Guilds.Recruit(this, c);
                        break;
                    }
                case GuildAction.PromoteMember:
                    {
                        if (data.Strings.Length < 1 || GuildId == 0)
                            return;

                        var name = data.Strings[0];

                        Game.Guilds.Promote(this, name, (GuildMemberRank)data.Data);
                        break;
                    }
                case GuildAction.DonateEMoney:
                    {
                        Game.Guilds.DonateCP(this, data.Data);
                        break;
                    }
                case GuildAction.DischargeMember:
                    if (data.Strings.Length < 1 || GuildId == 0)
                        return;

                    Game.Guilds.Discharge(this, data.Strings[0]);
                    break;

                case GuildAction.DonateMoney:
                    {
                        Game.Guilds.DonateGold(this, data.Data);
                        break;
                    }
                case GuildAction.QuerySyndicateAttribute:
                    SendGuildInfo();
                    break;

                case GuildAction.PromoteInfo:
                    Game.Guilds.SendPromotionData(this);
                    break;

                case GuildAction.QuerySyndicateName:
                    {
                        var g = Game.Guilds[data.Data];
                        if (g != null)
                            Send(CreateStringPacket(StringAction.Guild, g.Id, g.Name));
                        break;
                    }
                case GuildAction.LeaveSyndicate:
                    if (HasGuild)
                        Game.Guilds.Quit(this);
                    break;

                case GuildAction.SetAlly:
                    if (HasGuild && data.Strings.Length > 0)
                        Game.Guilds.AddAlly(this, data.Strings[0]);
                    break;

                case GuildAction.ClearAlly:
                    if (HasGuild && data.Strings.Length > 0)
                            Game.Guilds.RemoveAlly(this, data.Strings[0]);
                    break;

                case GuildAction.SetEnemy:
                    if (HasGuild && data.Strings.Length > 0)
                            Game.Guilds.AddEnemy(this, data.Strings[0]);
                    break;

                case GuildAction.ClearEnemy:
                    if (HasGuild && data.Strings.Length > 0)
                            Game.Guilds.RemoveEnemy(this, data.Strings[0]);
                    break;

                case GuildAction.ApplyAlly:
                    if (HasGuild && data.Strings.Length > 0)
                            Game.Guilds.AddAlly(this, data.Strings[0]);
                    break;
            }
        }
01/03/2015 22:58 Best Coder 2014#11
Quote:
Originally Posted by .John View Post
The packet parser it's self is a reflection scanner which scans all classes at start-up for packet handlers ( a bit more complex, but that's the general idea)
John's Overly Complicated Annotation/Attribute-based Observer Pattern™.
01/04/2015 10:35 Ultimation#12
Quote:
Originally Posted by Best Coder 2014 View Post
You talk about OOP, but you're breaking one of the fundamental priciples of OOP - encapsulation. Your beloved constructor is completely useless since you're publicly exposing all the internal fields of the class anyways.

These "functions as constructors" are actually an OOP concept as well. It's called factory methods. I'm not saying they're used properly in most CO pserver sources, but to say they're not OOP is wrong. An example of good use of a factory method can actually be found in TQ's CNetMsg class:


Not really related to OOP, but:
Speaking of Delphi/Pascal style, your [Only registered and activated users can see links. Click Here To Register...] is super frustrating. Sometimes you use PascalCase for parameter names, other times camelCase. What gives? At least be consistent. I know there's no official standard on this, but I don't think you'll ever find any serious people who'll tell you to use PascalCase for parameter names in C#.
Quote:
Originally Posted by Best Coder 2014 View Post

I don't see any read-only fields. I'm not disagreeing with you or Ultimation, I'm just pointing out some flaws in his example code.



I know, that's why I said: "I'm not saying they're used properly in most CO pserver sources". My point was just that this concept of "functions as constructors" - factory methods - isn't necessarily a bad thing in OOP.

it was an example from acidco about 4 years ago. things have changed since.

Now the point about constructors, yes the way i do it isn't perfect but it does keep things tidy for larger projects. why create a new class and then assign each required parameter. over time that may get somewhat annoying?

I personally prefer to do new Class(arguments) than new Class(){arguments}

Quote:
Originally Posted by CptSky View Post
Was checking the ItemUsagePacket (but with a second thoughts, I think PacketType should be a constant, and shouldn't be considered as a read-only field).
I disagree, its part of the IPacket interface.
01/06/2015 01:31 .John#13
Quote:
Originally Posted by Best Coder 2014 View Post
John's Overly Complicated Annotation/Attribute-based Observer Pattern™.
Of course :)

Code:
using System.Runtime.InteropServices;

namespace ProCS.Packets
{
    [PacketData]
    [StructLayout(LayoutKind.Sequential)]
    public class Guild
    {
        public GuildAction Action { get; set; }

        public int Data { get; set; }

        public int RequiredLevel { get; set; }

        public int RequiredRebirth { get; set; }

        public int RequiredProfession { get; set; }

        public string[] Strings { get; set; }

        public Guild()
        {
            Action = GuildAction.None;
            Strings = new string[0];
        }
    }
}
01/11/2015 01:30 Super Aids#14
Quote:
Originally Posted by .John View Post
As for packet processing, here is the latest packet management I added to ConquerPS. An automatic structure parser via Reflect.Emit (meaning it's blazing fast after the first call, no performance issues). It basically packs/unpacks packets via structs/classes. The packet parser it's self is a reflection scanner which scans all classes at start-up for packet handlers ( a bit more complex, but that's the general idea)

Code:
   [PacketHandler(PacketType.Guild)]
        private void OnGuildRequestPacket(Packets.Guild data)
        {
            switch (data.Action)
            {
                case GuildAction.ApplyJoin:
                    {
                        var c = Game.Players[data.Data];
                        if (c != null)
                            Game.Guilds.Join(this, c);
                        break;
                    }
                case GuildAction.InviteJoin:
                    {
                        var c = Game.Players[data.Data];
                        if (c != null)
                            Game.Guilds.Recruit(this, c);
                        break;
                    }
                case GuildAction.PromoteMember:
                    {
                        if (data.Strings.Length < 1 || GuildId == 0)
                            return;

                        var name = data.Strings[0];

                        Game.Guilds.Promote(this, name, (GuildMemberRank)data.Data);
                        break;
                    }
                case GuildAction.DonateEMoney:
                    {
                        Game.Guilds.DonateCP(this, data.Data);
                        break;
                    }
                case GuildAction.DischargeMember:
                    if (data.Strings.Length < 1 || GuildId == 0)
                        return;

                    Game.Guilds.Discharge(this, data.Strings[0]);
                    break;

                case GuildAction.DonateMoney:
                    {
                        Game.Guilds.DonateGold(this, data.Data);
                        break;
                    }
                case GuildAction.QuerySyndicateAttribute:
                    SendGuildInfo();
                    break;

                case GuildAction.PromoteInfo:
                    Game.Guilds.SendPromotionData(this);
                    break;

                case GuildAction.QuerySyndicateName:
                    {
                        var g = Game.Guilds[data.Data];
                        if (g != null)
                            Send(CreateStringPacket(StringAction.Guild, g.Id, g.Name));
                        break;
                    }
                case GuildAction.LeaveSyndicate:
                    if (HasGuild)
                        Game.Guilds.Quit(this);
                    break;

                case GuildAction.SetAlly:
                    if (HasGuild && data.Strings.Length > 0)
                        Game.Guilds.AddAlly(this, data.Strings[0]);
                    break;

                case GuildAction.ClearAlly:
                    if (HasGuild && data.Strings.Length > 0)
                            Game.Guilds.RemoveAlly(this, data.Strings[0]);
                    break;

                case GuildAction.SetEnemy:
                    if (HasGuild && data.Strings.Length > 0)
                            Game.Guilds.AddEnemy(this, data.Strings[0]);
                    break;

                case GuildAction.ClearEnemy:
                    if (HasGuild && data.Strings.Length > 0)
                            Game.Guilds.RemoveEnemy(this, data.Strings[0]);
                    break;

                case GuildAction.ApplyAlly:
                    if (HasGuild && data.Strings.Length > 0)
                            Game.Guilds.AddAlly(this, data.Strings[0]);
                    break;
            }
        }
Implemented something similar, but in D.
Code:
import packets;

@Packet(PacketType.test1)
static void handle1(Client client, DataPacket packet) {
	// ... Handle packet 1
}

@Packet(PacketType.test2)
static void handle2(Client client, DataPacket packet) {
	// ... Handle packet 2
}

@Packet(PacketType.test3)
static void handle3(Client client, DataPacket packet) {
	// ... Handle packet 3
}
And the implementation:
Code:
enum PacketType {
	test1 = 1003,
	test2 = 1004,
	test3 = 1005
}

struct Packet { // The Packet attribute ...
public:
	PacketType type;
}
All packets has to be imported public like the following:
Code:
public {
	import packets.testpackets;
}
And the packet handler is implemented like the following:
Code:
void handlePackets(Client client, DataPacket packet) {
	switch (packet.type) {
		mixin(getPacketHandlers);
		
		default: {
			// not handled ...
			break;
		}
	}
}

string getPacketHandlers()
{
	import std.string : format;
	import std.array : join, split;
	import std.conv : to;
	import std.algorithm : startsWith;
	import std.traits;
	
	string[] cases;
	
	foreach (phandler; __traits(allMembers, packets)) {
		static if (mixin ("isCallable!" ~ phandler)) {
			foreach (pattr; mixin (format("__traits(getAttributes, %s)", phandler))) {
				static if (startsWith(pattr.stringof, "Packet")) { // static if (is(pattr == Packet)) does not seem to work :/
					auto fname = split(phandler.stringof, "\"")[1];
					cases ~= format("case %s: %s(client, packet); break;", to!string(cast(ushort)pattr.type), fname);
				}
			}
		}
	}
	return join(cases, "\n");
}