Hot answers tagged

7

Welcome to CodeReview. If you are going to permanently keep your debug code around then you might want to you might want to embed it within ifdefs such as #ifdef DEBUG debug code ... #endif Generally, here in CodeReview, we frown on still having debug code within the source code. Your debug code could be optimized by using "\n" in the first 2 print ...


7

Well I learned a lot today, specifically that writing to UDP is on average faster than writing to a file on my local machine and on my servers. My implementation on the UDP client was off. If you look at this code: require 'socket' s = UDPSocket.new before_time = Time.now 100_000.times do |i| s.send("#{i}", 0, 'localhost', 1234) end puts "Time elapsed ...


7

I personally don't like sprinkling this all around the place. In generally I've found to use something like _ as prefix for private class members to be quite effective as a visual reading aid (like you've done for _endPoint). At least you should be consistent. It is not obvious what unit SERVER_READ_TIMEOUT or SERVER_LOOP_LIMIT are. You should either use ...


5

Overall - good initial post. Recommend to include the corresponding header file first. If heartbeat.h relies on any include file, then it (the .h file) should include them. View this from a user's perspective of the functions in the .h file. The user should not need to know .h perquisites, let the .h file include them itself. By including the .h file ...


5

General style Your general code style is good. The methods are named feasibly and you stick to PEP 8. Separate client and server shutdown Currently your "quit" command shuts down both the server and client. Since it is a command entered in the client to the server I'd expect it to either shut down the client (firstly) or shutdown the server, but not both. ...


5

UDP is not reliable. The packet sent to the sever could be lost (and therefore the server will not answer). The packet sent by the server could be lost. The client must handle such possibilities. As coded, it just hangs in recvfrom indefinitely. Your recvfrom only takes 1024 bytes. If the shell output is longer, the rest is irrecoverably lost. If the shell ...


4

def getInfo(): global info while 1: Don't use while 1 to symbolize an infinite loop; it does not look very pythonic. A better way to write this would be to use True. try: #This will bring the required info as pandas df info = retriveInfoFromDB() except: pass Don't just except; you should be catching a specific type of ...


4

sys.argv is already a list. I don't see a purpose of list(sys.argv). Creating sockets in the mainline looks strange. It is more natural to create them in corresponding __init__ calls. Same applies to serv.listen(). from socket import * is really bad practice, especially when socket is used as a parameter: self.socket = socket makes me wonder whether the ...


4

Constructors and Generics It is customary to use constructors to set values for instances in Java. This code: private class SendAndWaitForACK implements Callable<Result> { private CameraCommands cmd; @Override public Result call() throws Exception { // does not return on timeout result.dataBytebuffer = postCommand(cmd); ...


4

Client In __init__ you should have self.running = False and start should look like this: def start(self): if not self.running: self.running = True self.thread.start() Instead of self.lock.acquire() self.data = (a, b, c) self.lock.release() it's safer and more elegant to write with self.lock: self.data = (a, b, c) ...


4

Some notes: data as a name has been criticised elsewhere; I would simply say that any variable or property should be self-explanatory within context. a, b, and c should also have more descriptive names. You don't need self.ip and self.port in the server - they are only used in the constructor. YAGNI. You should default timeout to None and forward it ...


4

General From the code that you posted it looks like you are creating a client for every message that you are sending, this isn't very common behavior. Unless there is a specific reason i would expect the connection to be reused multiple times. I.e. at start set up the client and then use the existing connection to write as often as needed. Creating ...


4

General Classes should be final unless you intend and have designed them to be extended. Marking local variables as final will clue the reader in that they don't change once assigned, which improves readability. Use full, descriptive variable names. The extra characters don't cost you anything, and they make the code more clear and easier to read. The ...


3

You extracted "BBB" into a pseudo-const, which is good. However, you missed one: res = struct.unpack("BBB", data) This obviously should've been: res = struct.unpack(S_TYPE, data) If you aren't sure whether you replaced them all, Ctrl + F works in pretty much all editors. Often there's even a search and replace :-)


3

Even the question is so what out dated here some things I see on your code Copy Assignments You should remove the = on lines like MyClass foo = {}; and rather do MyClass foo {}; The difference is, that with the = First an rvalue will be instantiated, after that the move assignment operator will be called. This makes a difference in performance when ...


3

WRT intuitive CLI argument parsing, you might consider checking out the click library, if not for this project then for the next. Your CLI code might look something like this: import click @click.command() @click.option("--fancy-title", default=False, help="Utilise une barre de titre" " un peu plus Gtk3") @click.option("--serial-port", type=...


3

Improve consistency Duck-typing can be useful in some situation, but letting users that may want to provide their own reader for their own devices figure out the internals of your particular duck-typing is not very friendly. Look at how you define your reader in droneracer.py: if args.serial is not None: reader = drone_racer.XBeeReader( ...


3

At first glance it looks good with the one exception that Python code should be indented by four spaces, not two; but it's consistent, so there's that. Good job for the first "real" script! The main function is a bit big. In general it'd be nicer to split the argument processing off and use the argparse library(!) to remove the amount of boilerplate code. ...


3

Regarding the first issue, all network interaction will be slower than file interaction. To write to a file, you dont need to create packets with addresses, and you dont have to have another process setup to handle this data. Network interactions are slow, and go through the OSI model http://en.wikipedia.org/wiki/OSI_model Regarding the second issue: You're ...


3

Note that Endpoints.Contains(endpoint) will never be true, because you create a new IPEndPoint, and it does not implement IEquatable interface. In addition to @mjolka's description why you have a 50% CPU load, I would like to suggest how you can avoid it. Currently you have a central place that manages all sockets in one place. I suggest to do it ...


2

It's not surprising that CPU usage is at 50%, it looks like you're busy-waiting. while (Receive) { try { var readyChannels = ... foreach (var channel in readyChannels) { ... } } catch (Exception ex) { ... } } From Wikipedia, In software engineering, busy-waiting or spinning is a ...


2

Your code looks pretty good to me - I would only change some minor details. I'd replace UdpClient seq with UdpClient list, because lazy sequences can be subtle. In your code, this doesn't seem to be a problem, but they can cause unexpected GC issues (by capturing a reference) and tricky performance behavior (by re-evaluating things). I'd probably write the ...


2

Instead of while 1 it's more idiomatic to use while True. Similarly, 4 space indentation is the recommended Python style. It makes things a lot easier to read, especially as the indentation affects control flow. You should read the style guide for lots of details on how to make code readable and Pythonic. Does Server really need to be a class? It has only ...


2

So @SirPython covered a lot. I'm just wanted to mention: threads in python are (most of the time) not executed in parallel because of the Global Interpreter Lock. So you'll have to use stackless python or the multiprocessing module if you want true parallelism. Do mind that either conversion is not magical and you would have to modify your code (specially ...


2

First things first: you should check out C# naming guidelines and try to follow those. I'm leaning towards converting all of the IDatagram implementations in to structs Don't. You should read this page, which explains when you should use struct and when you shouldn't. The relevant part is: AVOID defining a struct unless the type has all of the ...


2

For the timeout detection, you could simplify your logic, by making use of the SetReadDeadline. Thanks to this, your timers are not needed anymore. Your monitor would then look like this: type monitor struct { Conn *net.UDPConn Kill chan bool } func newMonitor(port string) *monitor { return &monitor{ Conn: bindAddress(port), ...


2

@MX D is correct in that you should not be catching exceptions, unless you are going to do something about them. This code can and should be written without catch statements. private void DoReceiveFrom(IAsyncResult iar) { try { EndPoint clientEP = new IPEndPoint(IPAddress.Any, 0); int dataLen = 0; byte[] data = null; ...


2

I would rename IPPacket to PacketParser, and then create an IPacketParser interface that defines the parse method, but make it a regular method, not static. Then, I would create a TcpPacketParser class and a UdpPacketParser class, which would eliminate the need for your switch logic within the parse method, thus there would be no need to return the ...


1

I just have some small notes, as your program is overall very good: Why virtual destructor? You never actually inherit from Console. If you want to be on the safe side if someone inherits from it eventually, mark Console as final to prevent it being used as base class. Also, your destructor does nothing, and as such the default destructor should be used. ...


1

I would throw the code away and start over. I'd get exchanges working with small minimal test routines before evolving to classes/objects/libraries/etc. You don't need anything fancy, like queues and such, for a synchronous system that doesn't send a new packet until the previous one is acked. The code below is essentially a client-server setup. Included ...


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