Podcast #128: We chat with Kent C Dodds about why he loves React and discuss what life was like in the dark days before Git. Listen now.

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....


13

The lack of curly braces here has a potential for errors: if (argc < 2) /*TODO daemon */ cout << "Starting server\n"; else for(int i=1; i<argc; i++) if (!strcmp(argv[i], "-s")) shell(); else if (!strcmp(argv[i], "-s")) exit(0); else usage(argv[0]); The if statement may ...


12

I won't talk about the big picture (design), but here are some details where you can enhance your code: The "iterator" for loop was idiomatic a few years ago: for(std::list<Queue>::iterator it = listQueue.begin(); it != listQueue.end(); ++it) But now we have the awesome range-based for: for(auto&& elem: listQueue) It will do almost the ...


12

Some quick things that I can see : public ArrayList<Client> getClients() { return this.clients; } You're returning the implementation, you should almost always flavor the interface, in this case List. Hiding implementation is a good thing. try { this.loadWorld(); } catch (IOException e) { // TODO Auto-generated catch ...


12

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 ...


11

/** * When the backupCount reaches X, the player and world data will be copied to a different folder */ private int backupCount = 19; //start at 19 so that it will backup early Unused variable with a comment that speaks of functionality so large it probably requires it's own set of classes. What's going on here? Presumably, you have some code that you ...


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

You will gain noticeable performance and code clarity by using switch statement instead if()..else if(..) in processMessage method. I would also define constants as the class static final variables instead defining them per each call in method (or defined them in config file if preferable). It is easier to control and maintain them if define in one place. ...


10

Are you sure that &(int) { 1 } does what you want it to do? I'm not sure it does. Using malloc()/strcpy() is probably better handled with strdup() (which you've used elsewhere, so you're already okay with that). In webserver_handle_connection, you're using malloc() to allocate space for an unsigned long. Why? Declare a normal local variable, and pass ...


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 ...


8

If you really are planning on creating a thread for each incoming request just use and Executors.newCachedThreadPool(). As long as the incoming requests arent very often, this ExecutorService will create a new thread when you submit to it, but if a thread is idle and not being used, the service will reuse the thread instead of creating a new one. Edit: ...


8

Blocking I/O doesn't scale well Blocking I/O usually requires a 1:1 coupling between threads and streams. Thin clients can get away with using blocking I/O, because they're not going to have 100+ connections open. For servers with long-lasting connections, it's not workable. You've already noticed this with your memory requirements spiking. Consider ...


8

In total, a process in Unix systems can have a limited number of open filehandles. By default this is only say 1024 per process in Linux; all of the open sockets also consume file descriptors. Even if you optimize the system, then reading the file for each request will be costly. In this case, if you are really concerned about the performance, read the ...


8

Name your class properly without spelling errors CAsnycSocketServer should be CAsyncSocketServer. I don't know whatfor you have prefixed the class with C , if you did it because it is a Class, then you should consider to remove it. In the CloseSockets() method you already call Clear on the ClientSockets so the same call in the Dispose() method is ...


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 ...


8

ServerSocket self.handler The handler evaluation in __init__ can be accomplished with an or ternary operation. It's clearer to the reader as to what's going on. Also, the name could be changed to HandlerClass, since it represents a class rather than an instance: self.HandlerClass = handler or Handler self.handler logging The check against ...


8

Note: This review focuses on the use of C++, rather than the functionality. Naming: IMHO, the use of "do" at the start of function names is unnecessary and makes the code harder to read. The names would be fine without it (e.g. sendSuccess, sendError, createSocket all make perfect sense). Server: If the port must always fit in a 16 bit unsigned int, we ...


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

NSURLSession was designed from the ground up to be as similar to NSURLConnection as possible, so that those accustomed to using the latter will have no problem transitioning to the former. Meanwhile, NSURLSession has a lot of major advantages over NSURLConnection. First of all, we can get rid of all the code for dispatching on a background thread. ...


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 still find your code very C like (not C++ like). If I was going to use a word to describe the difference is that C is very imperative while C++ is more declarative. Example: Your code to tokenize the stream is: void tokenizeCmd(const string &str, vector<string> &tokens, const string &delimiters = " ") { string::size_type lastPos = ...


7

Cryptic variable names I had a hard time reading your code due to all of the short variable names such as cs, ss, scl, cal, ca, etc. It would be very helpful if you used descriptive names. Unsafe argument passing to threads Right now, your threads require two arguments, the file descriptors scl and fcl. The threads are getting these arguments from ...


7

Wrapping your methods in a class definition isn't the same thing as making your project OOP. These methods don't care about their receiver, and forcing the programmer to instantiate the PrintJobs class before calling them doesn't accomplish anything useful here. Now, for special-purpose scripts like this, not being OO isn't the worst thing in the world - in ...


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 ...


7

Learn to love rustfmt: Spaces go between comma-separated items: -use std::io::{Read, Write,BufReader, BufRead}; +use std::io::{Read, Write, BufReader, BufRead}; -let response = format!("{}{}{}{}","HTTP/1.1 ",status," OK\n\n",res); +let response = format!("{}{}{}{}", "HTTP/1.1 ", status, " OK\n\n", res); Spaces go before braces: -fn main(){ +fn main() { ...


6

Here's an example adapted to your case which uses ThreadPoolExecutor. This example allows a configurable throughput while establishing TCP connections to servers. Checking Future.get() for each submitted worker thread ensures that no servers are checked more than others. What happens here: Target domains are added to a list If the domain is an unknown ...


6

The ugly part that sticks out is that you are silently ignoring the exceptions... You should do some cleanup and close the sockets at least. Improving a bit on the performance side, you are starting a new thread to execute a very small task, thus wasting a lot of time to start-up threads. You could instead submit the Runnables to an ExecutorService, ...


6

In addition to Tudor's response, you also shouldn't write infinite loops - that's just silly. For servers that are intended to run for an unknown amount of time (possibly endlessly), I do while (keepServerRunning) { // where 'keepServerRunning' is a simple boolean variable ... ...and then use a shutdown hook to set the value to false which allows the ...


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 ...


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