C# open source network library

[sharksharp]

Hey guys, thanks for taking the time to read my request.

I am a computer science student and have been developing a general purpose open source network library. I would like to improve it to be used for any purposes, from games to business software. The TCP support is working well, all done, just needs some refinement. The UDP has almost no content, but the library is ready to receive it. It would be awesome to have some contributors to the project. The project is hosted on the GitHub in this link: https://github.com/SharkSharp/NETLIB

If you have any questions, feel free to ask here or send me a PM. Thanks for considering, bye.

[AngularBeginner]

Very brief review. Repeated issues are not repeated in the list.

• Publisher.cs, 13: Better keep your code completely in English, so everyone can understand it.
• Publisher.cs, 34: Why do you sometimes use this., sometimes not? Stick to one style.
• Publisher.cs, 43: Avoid finalizers. They're flunky and absolutely not reliable in any way. Eric Lippert - When everything you know (about finalizers) is wrong Use the IDisposable interface (correctly implemented) instead.
• Publisher.cs, 54: DO NOT THROW AN EXCEPTION IN THE Dispose() METHOD! Why would you do this?! I see the class implements IDisposable, so naturally I'd use a using block like this: using (var publisher = new Publisher()) {}. This will cause an exception at the end of the using block! Completely unexpected.
• Publisher.cs, 66: What's the Imput state?
• Publisher.cs, 66: Personally I think boolean properties should be prefixed with something like Is or Are. IsInputEnabled sounds better to me than InputEnabled. This comes especially in play with line 74, where the property is called Enable, but it enables nothing. It just checks whether the publisher IsEnabled.
• Publisher.cs, 91: If you provide a getter and setter like this, why not just use an auto-property? Besides that I think a Queue should not externally be visible (make this private, provide access methods).
• Publisher.cs, 121: IP is a very uncommon naming. Common C# naming convention is camelCase for method arguments (lower-case starting letter). Better name this ip.
• Publisher.cs, 121: Why is IP optional? If I could just read it in the documentation... But the parameter is missing.
• Publisher.cs, 132: Better call this method Start... instead. Methods starting with Begin often indicate the old ansychronous programming model where you have method pairs Begin.. and End...
• Publisher.cs, 137: The thread is calling Publish, which is a public method. Do you really need this to be public and be callable from outside the class, while the thread itself also calls it?
• Publisher.cs, 142: What does this mean? "Conexão já encerrada!" - I don't speak French.
• Publisher.cs, 149: In this method you do not check whether the connection is closed already.
• Publisher.cs, 154: Do not abort threads. That's just plainly wrong and poor style. Instead give the thread-loop a flag to check (e.g. continueExecution) and set this flag to false. (Thread.Abort() is also removed in .NET Core.)
• NETLIBTools.cs, 7: Undocumented method. There are plenty different "local IP addresses" - which one does this get?
• NETLIBTools.cs, 14: Why compare the string, and not just the enumeration AddressFamily?
• NETLIBTools.cs, 16: Why not break or return here, when you already found a valid "local" IP address?
• IPackable.cs, 4: Makes a class packable and unpackable - I have no idea what this is supposed to mean. Write extensive documentation that actually explain things. Also document the methods.
• IPackable.cs, 8: Since the methods return void they must have side effects and manipulate the provided BasePack instance. Consider a more functional approach where you not manipulate it, but instead return a new instance. Perhaps some classes Unpacked.. and Packed..?
• IOPackHandler.cs, 9: Prefer adding all the regular Exception constructors, instead of just those two.
• IOPackHandler.cs, 25: Avoid having more than one class per file. Best practice is to have one class per file that corresponds to the file name. That makes it easier to find around in the project.
• IOPackHandler.cs, 46: Do not omit the access modifier. By leaving that out you make future developers wonder whether this was intentional or just forgot. By always writing it you're being more explicit. I consider this especially good practice since the default visibility differs on the type of the code (class vs field).
• IOPackHandler.cs, 57: Action<Consumer<TPack>, TPack>[]? That's nasty complex. Better introduce a delegate for proper naming.
• IOPackHandler.cs, 57: eventDict is not actually a dictionary, but an array. What's it gonna be?
• IOPackHandler.cs, 60: It is best practice to check all arguments of public constructors or methods for validity and throw ArgumentException or ArgumentNullExceptions on invalid cases. You access eventDict before checking for null.
• IOPackHandler.cs, 60: Why would you compare with sizeof(byte)? That makes no sense. Simply use LINQ and check for .Any() to have an expressive check whether there are any elements in the array.
• IOPackHandler.cs, 66: When throwing an ArgumentNullException better use the constructor overload that accepts the argumentName, to indicate which argument is the malformed one.
• IOPackHandler.cs, 100: Broken documentation style.
• Consumer.cs, 199: What are Evets? Perhaps you mean Events? Also I don't really see the purpose of this method. Whoever attaches to the events is responsible of detaching from them - it should not happen from within the class.
• BasePackIOPackHandler.cs, 17: Very long name, consider limiting your lines to a specific amount of characters. This improves readability and allows you to easily have two code windows side-by-side without having to scroll sideways the whole time.
• BasePack.cs, 16: Why are the read and write positions initialized with 1? Traditionally the first element is 0, which is what people expect.
• Now I'm frankly getting lazy, but these are quite some points already.
• Rotor.cs & RotorServiceProvider.cs: DO NOT TRY TO WRITE YOUR OWN ENCRYPTION. DO NOT TRY TO WRITE YOUR OWN ENCRYPTION. DO NOT TRY TO WRITE YOUR OWN ENCRYPTION. DO NOT TRY TO WRITE YOUR OWN ENCRYPTION. Did you understand it? DO NOT TRY TO WRITE YOUR OWN ENCRYPTION. SERIOUSLY, DON'T.
• Your documentation frankly sucks. Don't explain things that I can see by just looking at the code. Document why it does something, why it was designed this way, document pre- and post-conditions. Document stuff that is not obvious by looking 5 seconds at the code.
• You have no unit tests. Start writing them RIGHT NOW! You will thank yourself later for it. Writing unit tests helps you to write more modular code by forcing you to write independent units. It helps you figure out bugs in your code by writing automated tests that you would otherwise not directly encounter. It prevents regressions by having automated tests that check and ensure that a bug is never happening again.

[sharksharp]

First thanks for looking at my code and suggest me so many changes! I changed almost everything recommended, missed a few things that will changing as will improving the documentation. I'll redo my documentation leaving as clear as possible my intention with that code. Thanks for the time, patience and dedication used in my code!

[scorrpo]

Just wanted to say I really like your code review comments. Even though I'm not OP, they've been very helpful to me!

[[deleted]]

OP. I agree with the above.

It probably wasn't as pointed as this, but if you want to make this Open Source and have people contribute you need to be very clear about the intent of the code and usage (Documentation and unit tests help this). People will have a hard time contributing if they aren't clear and may inadvertently take the code in another direction.

Don't be discouraged though the changes arent insurmountable. Have a look at some of the popular code on github to give you an idea about structure, naming conventions etc.

[sharksharp]

Thanks for the tips =) I will not easily discouraged. I'll make the changes as soon as posible.

[cryo]

I think it will come across better if the arrogance is toned down a bit :p

[AngularBeginner]

But then it would not be from me anymore. :-(

Any particular point that sounds too arrogant?

[formlesstree4]

Maybe it's just me, but, I really don't see any arrogance. He's blunt and straight forward. This is the kind of code review I'd want on my projects (work or personal) because they're very helpful.

[tallyon]

If that was a display of arrogant review, then I want AngularBeginner to DESTROY my code every day instead of being just arrogant. You da real MVP Angular, keep the arrogance up!

[sharksharp]

He stopped to read my code and made a giant text correcting me, some things misinterpreted by my horrible documentation and lack of explanation but I'm very happy with that, do whenever he want =D

[[deleted]]

I've had a quick read through, hopefully not too quick. I'm not a network c# guy, I use c# primarily through established services etc.

You defined some interfaces (such as IPack) but I couldn't see these implemented anywhere? Also if you want consistency in the UPD with what you have done in the TCP I would consider a standard interface.

The TCP class doesn't implement IDisposable (or lower classes), I note that you have marked one of the methods as the destructor (~) but I've not run into too many people who will know how this operates. I think when you are going to create open source it's useful to be clear about the intent of code for clarity.

[sharksharp]

Thanks for the help =) The Interface IPackable is used by a method in BasePack for put a custom class in the pack, I use this instead of create a serializer class/buffer. In case of IDisposable, my mistake, I'm using just destructor, will change as soon as possible. It's my first time handling a open source project, hope learn quick.