Hot answers tagged

15

I can't resist saying this… the way you use a lot of whitespace to achieve your horizontal alignment, especially in server.cpp, is distracting and hinders readability. I don't mind it so much when several lines have some relationship to each other. For example: serverAddr.sin_family = AF_INET; serverAddr.sin_port = htons(8080); serverAddr....


14

Code Review A standard naming convention (if there can be said to be one). Is that user defined types start with an initial capitol letter. Other identifiers that name objects (or functions) start with an initial lower case letter. This lets you find user defined types very quickly in the mess that is the C++ syntax. You seem to have all the variables (...


13

This won't be comprehensive review/ramble, because I'm hungry; however, it is nice to see fairly comprehensive inline documentation on the public API! General Networking Commentary Because you asked if there are better ways of doing this, here is some high-level discussion that doesn't relate so much to your precise code. I've written code like this dozens ...


12

Observations as I'm reading down the code: Server Class doesn't implement an interface. If I'm using your library, at least one of my classes is going to have a dependency on the Server class. If I use an IoC container, I need to pick one that can work with concrete types (that's usually not a problem though). If I want to write unit tests for that class ...


11

A few notes: As Jamal noted in the comments, your definitions of STDIN and STDOUT are somewhat useless. You can see here that using the given STDIN_FILENO and STDOUT_FILENO are already given those values. To counter your claim in the comments that they help readability, I would argue that they actually clutter up your code and should be removed. Declaring ...


11

One of the key points in having readable and maintainable code is to have consistence of the choosen style which isn't the case in your code. You have different indention styles for methods. Sometimes you are using K&R and sometimes you are using Allman style. sometimes you are using underscore prefixed method variables (which isn't the preffered ...


11

One of the issues with TCP sockets is that there's no guarantee that sending N bytes in one go will result in receiving exactly N bytes after one call to read(). I would thus emphasize this by using a separator in the messages (newline perhaps?) and only printing out the result of what I've read after the whole line has been received. I'd also try to get ...


10

I don't see why you say <- "decreases readability", but you could line them up if you think that helps (this style is typical of Haskell programmers) main = forever $ do diff <- user1 `subtractCounts` user2 timestamp <- formatTime defaultTimeLocale dateFormat <$> getZonedTime homeDir <- getHomeDirectory let logMessage = ...


10

I'm not a C# expert, so I'll try my best to give a good review: {"chanlist", "Provides a list of channels joined."}, {"channels", "Provides a list of channels joined."}, These functions are identically described, either make them different or remove one. recipient: you don't need this field. It's only used in one function, and it could be ...


9

0: Keep class members private unless there is a darn good reason to expose them. And then, if you have to, use properties. 1: Use readonly on class declarations which are considered unmodifiable after construction. Example: public readonly ManualResetEvent connected = new ManualResetEvent(false); This declares intent and keeps other code from accidentally ...


9

Detect closed connection 1 select promises that a read wouldn't block. This is a case when there are data, but this is also a case that the remote end is closed. In this case read immediately returns 0. Without testing for len > 0 in transfer end up in an infinite empty select/read cycle. Detect closed connection 2 The remote may close connection while ...


8

I don't understand the point of the IComponent / IComponentConsumer stuff. Nothing else in the large section of code you've posted uses them. Are you sure they're at the right layer? Also, with IComponentConsumer, I think the code falls into the trap of extending when it should compose. It seems to me that every class which implements the interface will ...


7

Using imports: catch(java.net.SocketException se) { /* [...] */ throws java.net.SocketException { /* [...] */ fully qualifying these is unnecessary: import java.net.SocketException; catch(SocketException se) { /* [...] */ throws SocketException { /* [...] */ I feel that this code is much more concise, while containing the same information. Using try-...


7

I'm not an expert in the library or multithreading, so maybe some advises will not be applicable. TL;DR I didn't find anything in your code really wrong. In fact, the more I look at it, the more I can just find only nitpicking things, and I'm really forcing myself. I would find myself quite happy to maintain this code. Everything is clean, the exception ...


7

There is a lot of ground to cover here... General ToString You do not have a toString() on your JSONClient code. These are invaluable for debugging, etc. Configuration There are a lot of values which are hard-coded in your program. Things like IP addresses to bind to (you have the 'wild-card' 0.0.0.0 hardcoded) can be specified on a per-interface basis, ...


7

Your vertical separation is good, you've got clear regions in your code for defining the objects you need like sockets, readers and writers, etc. There's one region that needs cleaning, though: while (p1.isClosed() || p2.isClosed()) { // if one of the players disconnect, the match will end. try { String p1 = bos1.readLine(); // what p1 says ...


7

Using semicolons is OK. The IO is imperative and side-effectfull by default, so there is nothing to do here. By saying Lwt_unix.listen sock max_pending_request you're just setting a property inside the kernel. It is not blocking, so there is no need to create a thread here. However, you should never use ignore function, to cast away the Lwt.t type. This is ...


6

You are holding on to some IDisposable resources that you shouldn't be, which will impact scalability, GC pressure and likely performance. Here's the augmented Listener class: internal sealed class Listener : IDisposable { private readonly int port; private readonly TcpListener tcpListener; private readonly IList<Client> clients = new ...


6

dead code should be removed the indention is horrible byte[] StreamMessage = new byte[9632*2]; try { socket = peerListener.AcceptSocket(); Thread.Sleep(500); MessageLength = socket.Receive(StreamMessage, 0, StreamMessage.Length, SocketFlags.None); } should be look like byte[] StreamMessage = new byte[9632*2]; ...


6

A few of the class names are unclear. ServerListener sounds like it listens for some action by the server. Instead, it is the code that runs on the server listening for connections from clients. Authentication doesn't do any authentication, it is just a user's login credentials. Most of your classes are Runnable, but they encapsulate the thread that they ...


6

This may not be useful for accounting for invalid command line arguments: assert(args == 3); It's more common to inform the user of the correct arguments upon failure. This message should be printed to stderr and the program should terminate with some valid error value. Here's an example of this: if (argc != 3) { fprintf(stderr, "usage: %s arg1 arg2"...


5

Ohhh... a juicy question. Your specific concerns are about the GET calls, but let me address some other things first.... General You have put your entire server-socket loop inside the constructor of the server. Server socket loops are never pretty, and I can understand your uncertainty of how to handle things, but I would suggest one of two things: set up ...


5

White space and Casing is good. Just a few things to think about: #region is not very well accepted. If you have to use #region, then you should look at moving the code out into a method or its own class. Use var instead of explicit declarations for obvious variables. This makes the code much easier to scan over. If you insist on comments to explain what ...


5

(It's not a complete review, just a few random notes.) ArrayList<...> reference types should be simply List<...>. See: Effective Java, 2nd edition, Item 52: Refer to objects by their interfaces private static List<String> users = new ArrayList<String>(); Collections.synchronizedList() does not modify the parameter. It returns a new ...


5

I only quickly glanced at your code, so this isn't going to be a thorough review, but I think the basic idea is surprising: Service never returns null or throws exceptions. I think this approach is error-prone. When I call a method called GetUserById, for an Id that doesn't exist, I expect either an ArgumentOutOfRangeException (or similar), or a null ...


5

The warning is trying to tell you that the return is pointless, because it's the last statement anyway. Here's a better way to rewrite that, which will make the warning naturally go away: if (!pwd.equals(pwd1)) { onShake(edt_password, "Passwords do not match"); onShake(edt_password1, "Passwords do not match"); return; } startbackgroundtask("...


5

First off, don't use 1 over True in while loops. While 1 works, using True is much clearer. Secondly, you have inconsistent indentation. While in Python 2.x, this doesn't affect much, Python 3.x will throw an error. Never use try-except without a specific error to catch. There could easily be an error that goes un-noticed. Most of your names are okay, but ...


5

Some issues that occur to me: The obvious problem is that there is no error checking. memset is often preferred to bzero, although I don't see why. Also the cast of serverAddr to char* in the bzero call is unnecessary. port number should be shared in a header file is inet_addr the correct way to get the address? Also note that it, too, can fail. And there ...


5

A public non-virtual destructor in a base class is risky. You should consider making it protected, or otherwise virtual, if there's need for polymorphic deletion of the BaseSocket. I don't think 0 is a good value to indicate an uninitialized socket id. The socket function will return: Upon successful completion, socket() shall return a non-negative ...


5

Creating a session As a library, I find it barely usable. If I want to use it with my own credentials, I have to: either modify the source of your library to put them in __init__; or tamper with the attributes after building an ArcsightLogger object: o = ArcsightLogger() o.login = 'spam' o.password = 'eggs' o.main() Moreover, having to manually store and ...


Only top voted, non community-wiki answers of a minimum length are eligible