The Stack Overflow podcast is back! Listen to an interview with our new CEO.
47

Overall this is quite a nice module. Here are a few usability issues/nitpicks, though: When running the script, there is no easy way to stop it. CTRL+C does not work, I have to manually kill the process. This is probably because of how threading.Thread handles it, but I'm not sure. It would be nice if it was slightly easier to add a different handler than ...


32

This is more of a design / algorithm / architecture review than of the code. There are some major issues the other answers didn't address. First of all, wasting network bandwidth running a speed-test on repeat seems like a bad idea. It will fill up your connection every hour (by default), so it hurts you personally if you happen to be doing something on ...


30

Checking TCP ports and multi-threading Your performance should be fine for a sequential program. Another way to check whether the TCP port is open is by just sending the initial SYN and wait for the ACK. That way, you don't have to do the complete TCP handshake. However, that also means that you have to implement that part of the TCP protocol yourself. Also,...


23

You may want to look some other guides/implementations about that: CodeProject | C# SocketAsyncEventArgs High Performance Socket Stack Overflow | How to write a scalable Tcp/Ip based server About your code: You should seperate the UI and the server code, so you can be able to use it on a console application or -more likely- a windows service. ...


22

I'd like to say just one (yet IMO very important) thing: You're developing bad habits early! The point of classes is to eliminate (truly) global state and to manage it within classes and objects. You've really abused the use of global which is making your code harder to digest. There are cases where it's fine to use globals, where the generality of passing ...


19

From perusing your code a few times: Custom event abuse While it's very cool to have custom events, I would simply remove this extra layer of logic. It does not make sense to call $(networkInfo).trigger('selectNetworkRanges'); if you could just call selectNetworkRanges(). I understand that you would loose access to this but you are accessing networkInfo ...


16

The first thing that stands out to me is that you've created an entirely new class that exists purely to have this class method. This doesn't make a whole lot of sense. At a minimum, this method could just exist on its own, outside of a class, but I'll do you one better: let's write it as an extension to UIDevice, that's where it makes the most sense, and ...


15

Concept Internet "speed" actually consists of multiple factors, including bandwidth, latency, and packet loss. What exactly are you trying to measure? If it's bandwidth (data transfer speed in bits per second), then you should also measure transfers of larger payloads, because the time to complete small transfers will tend to be dominated by latency ...


14

There are quite a few points of improvement in this code. Networking is not my speciality, but I can give you several suggestions on code improvement. First, a quick word on naming: Type names in ALL_CAPS (i.e.: SERVER) are a bad choice. Most programmers associate this kind of naming with constants and macros. For C++ types, you should probably use ...


13

I will not comment on the actual TCP functionality itself. I am not competent for that. Stopping broadcast - on purpose? When you are broadcasting to the clients in SendMessage(), it seems like you stop broadcasting if you get problems reaching one of the clients. Is this intended? I would move the try/catch inside the foreach, and around the if. Then, in ...


13

It seems fine if there is not any simpler Java or Apache Commons API. I'd modify a few small things: The isValid boolean would be unnecesary if you return immediately when you know the return value. (Flattening Arrow Code) (address == null || address.trim().isEmpty()) could be changed to StringUtils.isBlank. According to the Code Conventions for the Java ...


13

Concept I don't believe that there is a cross-platform way to read the ARP cache. From the use of /proc filesystem, I deduce that you are targeting Linux. In Linux, you could read /proc/net/arp as you have done, or run the command ip neigh, which does something similar to your program. (On OS X, you could run arp -a -n instead.) A comment worth a ...


13

There's lots of place for improvement. retry: // (1) try { if(getaddrinfo(NULL,INIT_PORT_NUMBER,&hints,result) < 0) { DWORD error = GetLastError(); throw new ERR(error); // (2) } freeaddrinfo(&hints); }catch(ERR& e) // (3) First (1), don't use goto in C++. There are (arguably) places where it improves things, ...


13

you should separate the IRCBot from the Main into its own class with it's own fields/properties you should specify all required parameters via the construtor so that it is not possible to create an invalid ircbot I don't think it's a good idea to make the retry recursive - IMHO it would be better to create a loop and limit the retries so that you don't have ...


12

Exception safety Your locked_queue isn't exception safe. In particular: queue.pop(); return value; If the copy (or move) constructor for _T throws, you could have popped the item from the queue, then the constructor throws as you return the value, and the value is lost and can't be recovered. This is exactly why the standard library separates ...


12

One-shot answer: private boolean isValidPublicIp(String ip) { Inet4Address address; try { address = (Inet4Address) InetAddress.getByName(ip); } catch (UnknownHostException exception) { return false; // assuming no logging, exception handling required } return !(address.isSiteLocalAddress() || address....


12

Server private static String names() { StringBuilder nameList = new StringBuilder(); for (String name : userNames) { nameList.append(", ").append(name); } return "In lobby: " + nameList.substring(2); } The above can be simplified by streaming and collecting with Collectors.joining(CharSequence, CharSequence, CharSequence) using ...


12

In addition to t3schb0t's answer I'd also change your parsing logic. Your code is just few lines and it's already too complicate to read. After you moved code into a separate class then it's time for refactoring! Abstract away read/write/log logic. You already have StreamReader, StreamWriter, Console and TcpClient. Current implementation is hard to test (...


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

There are a number of naming issues here so it's quite hard to understand the code, I'll start from those: public properties/events/methods should be named PascalCase (receiveMessageHandler => ReceiveMessageHandler) events are named according to the time they occur. If event describes that some fact occurred, it should be named in passive voice without "...


11

A few notes: Overall, you are doing a lot of unnecessary work. You could just use getifaddrs() to get the IP address, MAC address, and interface name. There are plenty of code examples online on how you can use it to obtain all of that information, including an example in the function documentation itself. You could also read directly from the file /sys/...


11

Argument handling $defaultmail and $defaultip are poorly named — there is a naming inconsistency: $host ||= $defaultip; … $mailto ||= $defaultmail; I would also advise against introducing $defaultmail and $defaultip as variables at all. Either just write them in directly: my $host = (shift @ARGV) || '8.8.8.8'; # Google's nameserver my $...


11

Here are some of the notes i jotted down while reading through your code: If you're going to make a copy of a string that is passed in by const reference, you might as well pass it by value and then move it. For example, in split: std::vector<std::string> split(std::string str, const std::string& delimiter) { // Just use str directly without ...


11

You can have more classes Your start(Stage stage) is doing a bit too much for what I like to see. You're creating an Anonymous Class that encompass the logic of reading the socket and the presentation of to the client. I would create a separate class that would extract that logic. This would need to pass the socket and the textbox, but I'm sure there is an ...


11

new Thread(this).start(); I may be wrong about this, but I don't think this is recommended in the constructor. ExecutorService implementations exist to provider better/consistent handling of asynchronous tasks. Arguably, a better form will be to extend your abstract class and use it with an ExecutorService instance, instead of having it (Connection) decide ...


11

I'll proceed top to bottom. mail_error Which would you rather read: x.body = e.message or mail.body = exception.message? Don't try to save a few characters on naming — your fellow programmers will like you better. In particular, x has a connotation of being a floating point number, which this isn't. scanning_all_servers and server_status Net::SSH#start ...


10

@KerriShotts already mentioned Whitespace in declaring your string arrays, but what no one talks about is the trailing (,) comma in your string array string[] errorArray = new string[] { "Device is working properly.", "Device is not configured correctly.", "Windows cannot load the driver for this device.", "Driver for this device might ...


10

Server I think you are over-using the static keyword. Don't make each and every function static, create an object of the class and use the object instead. You have some catch parts that are either empty or print very little information. I suggest you investigate a bit more about the different reasons those exceptions can occur and add some comments about ...


10

Nothing would make a reviewer happier than help you jam the networks of an innocent neighborhood. (sarcasm) Mutually exclusive conditions Can line match Address, Channel, etc at the same time? If not, then the multiple [[ ... ]] are needlessly evaluated, and it would be better to rewrite them using elifs: if [[ "$line" =~ Address ]]; then m=${...


10

There is no guarantee that write writes out the entire buffer. This means that if (FD_ISSET(STDIN_FILENO, &fds)) rv = transfer(STDIN_FILENO, buffer, BUFLEN, sd); may lose data. Consider looping until all data gone out. As long as the portability is concerned, the timeout parameter of select needs attention. The BSD implementation doesn't change it, ...


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