23

Networking You have done the classic thing that all people new to TCP do of assuming that whatever NetworkStream.Read gives you will be some meaningful chunk of data: this is not guaranteed, and only 'works' because you are sending tiny packages locally. Basically, if you want this to be a general-purpose and reusable system for sending discrete messages (i....


12

I have some ideas about how you might be able to improve your program. Avoid problems Rather than trying to deal with the problem for every instruction, one approach is avoiding it entirely. One way to do that is to simply append a number of bytes to the end of the vector. If the maximum bytes for an instruction is \$n\$, then append \$n-1\$ bytes to the ...


11

Python already has an exception that denotes that the value you passed is inappropriate somehow. It is ValueError, which is what the built-in int also raises if a wrong string is passed. In addition, defining a nice readable error which you can raise, only to catch it directly within the function and to return None (the output of print) and print to the ...


10

There is one issue with your approach which is, in the case of multiple import failure, only the last one can be properly reported: >>> try: ... from .foo import Foo ... except ImportError: ... from .base_engine import Default as Foo ... setattr(Foo, 'name', 'Foo') ... >>> try: ... from .bar import Bar ... except ImportError: ... ...


10

Instead of checking with an if statement if errStat is null and then returning false: if errStat != nil { return false } return true you can return a Boolean expression: return errStat == nil


10

Review This review handles readability metrics and C# conventions only and should provide insights to reach some of your goals. Goals: learn C#, improve coding style, create good to read code There are a couple of variants how to name instance variables. The most common one is to use an underscore as prefix and camel-case the name; _client, _listener, ...


9

Readability first I've tried to condense everything to one line, but am unsure if this is the most efficient way of approaching this problem. Your primary goal when writing code is readability and maintainability and not squeezing everything into single lines. Can you read this? return exception != null ? string.Format("{0}: {1}\n{2}", msgCount, ...


8

This is how I see things... Throwing ArgumentException insinde a try/catch that at the same time swollows it is pointless. You use ArgumentException to inform the user that he called your API in a wrong way. This is not something one should hangle because he won't see it until he looks at the logs. The app should crash in this case. The message of the ...


7

This answer builds upon the solution presented by Mathias Ettinger If you want to get rid of the try: ... except: ... boilerplate you could implement some kind of custom loader for your engines. This is something I wrote quickly, which is likely far from perfect, but it works. in __init__.py: import importlib def mock_missing(name, msg): def init(...


7

That's an odd use of the word "default", which, etymologically speaking, means to remove failure, and generally refers to a fallback setting that "just works". I suggest calling it Missing instead.


7

Just wanted to add that you may want to consider using Polly instead of rolling your own retry mechanism - this is a library built specifically for this kind of retry mechanism (as well as many more complicated scenarios). Using Polly would look something like the following. Note that I have renamed the attempts parameter to retryAttempts to make it clear ...


7

Exception If your algorithm cannot find a date, it is easier to raise an Exception than to return ''. Returning sentinel values instead of exceptions can lead to unexpected behaviour if the user of this function does not test for this sentinel value. comments Comments should explain why you did something, not how. # Take the last four digits tells you ...


6

I have a few suggestions: \n\t\t\t\t is everywhere, move it into a function which takes and returns a string, and prepends whatever string it is given with this format - the benefit of this is that if you change your formatting in the future, you only need to do it in one place. Getting the user to enter their ID is a common operation, so put this logic all ...


6

I think the big switch is a problem. Consider a more data-driven approach where each opcode is described by a struct: struct OpCode { unsigned char command_byte; unsigned char mask; // if only a subset of bits determine command unsigned char length; // other members as needed - e.g. a pointer to the "print" function }; Now, the code that ...


6

Whenever you find yourself trying to replace a standard language feature, the first thing you should think is, There's probably something I'm missing. What's the standard approach to this kind of problem? Your code feels inelegant because you're not using a standard approach. When your code starts to feel weird like that, it's time to take a step back ...


6

After our initial comment-review-update sequence/loop there is not much left for a review but still, one more thing. You can get rid of the if entirely when you add a when filter to the excpetion: catch (WaitTaskTimeoutException) when (attempts > 0) { //add some delay await Task.Delay(10000); return await ...


6

ClassLoader.getResourceAsStream() throws a NullPointerException if the given name is null, something you can easily check. InputStream.readAllBytes() does not throw a NullPointerException, and neither does instatiating the String. So basically, the null exception will happen if and only if fileName is null, or if the resource is not found and therefore ...


6

A try catch block is there to "Catch" a condition, some conditions you can solve yourself in code, some need to bobble up the stack to the user in form of a message, some end the application. A sample for user exception would be that a transaction crashed due to a network transport layer error, never becouse the user entered 0 to withdrawal. If however you ...


6

Error handling One specific question I have is if it's bad practice to return a print call, as a way to print an error and simultaneously return from the function? I did that so that I could print the error and return None on the same line. That is unconventional to say the least. By catching the exception internally and print it to the console you ...


5

The class is doing too much: it is not the task of the Account class to also keep a database of all accounts. Remember: one class (or function), one responsibility. By the same principle, menuSelection does too much in your main program. Use the initializer list in your constructor, i.e., do Account::Account() : name_(), id_(0), balance(0) {}. But even ...


5

Thank you for sharing your code and this idea with us! :) I have just some personal preferences and ideas to some of you "major problems" witch should be additional to the answers of @Mateusz Stefek and @TorbenPutkonen Not Functional public Try<T> catching(Class ex, Function<Exception, T> handler) { if(/* ... */) { throw new ...


5

Review Property Connected is not thread-safe. Use Interlocked.CompareExchange to get the connection atomically. Events DataReceived and Disconnected are never cleared. This will cause a memory leak. Implement a Clear function and/or implement IDisposable. Methods Send, Disconnect, EndReceive are not thread-safe. Consider using a mutex/lock. Checking ...


5

Elegance is the wrong criterion. The key criterion is correctness, which implies readability and maintainability. As you read text, you notice misspelled words. Does that mean that you laboriously spell-check each word, letter-by-letter? In fact, most likely, you quickly and unconsciously scan the shape of the words. Only if the shape of a word seems odd do ...


5

Throwing custom exception I find the second version is cleaner as it clearly communicates which condition caused the exception. With the first one, you need to know it's about Single which you need to read the documentation for. Implemention However, as far as the implementation logic is concerned this is doing a lot of querying and the source baseFilters ...


4

When VERBOSE=0, the following code: bool(os.getenv('VERBOSE')) still returns True. An alternative solution would be: bool(distutils.util.strtobool(os.getenv('VERBOSE')))


4

I would not recommend this pattern. I see no compelling reason to encumber NetworkRequestError (and presumably every other Error enumeration throughout your project) with this finish cruft. So, instead of: enum NetworkRequestError: Error { case hostNotAvailable case accountNotAvailable func finish<T>() -> Result<T, ...


4

My attention is really drawn to your error-codes script. I love a good lookup array, and you almost have one, but it needs some polishing. Rather than declaring the lookup variable with global to put it in the function's scope, realize that you probably aren't going to be modifying the data during processing, so it is a perfect candidate for being a ...


4

Review If the purpose is to output each of the descendant inner exceptions, you are missing all except the first inner exception in case of an AggregateException. And this is a common exception. Don't use \n as new line unless you specifically need to comply to this style of new line. Prefer Environment.NewLine. I am not convinced of the exception != null ->...


4

Note that some messages from exceptions expand to more lines (e.g. ArgumentNullException), so consider to replace new line characters while formatting the message so each exception message is written neatly in one line: exception.Message.Replace(Environment.NewLine, " ") My version would look something like: public static string GetMessageTree(this ...


4

BmiCalculatorWorking This name is confusing. All code should eventually be 'working'. Don't use static for all variables. I can only guess you did this to avoid the warning about calling non-static variables from a static method. Instead, you can instantiate a BmiCalculatorWorking, and call the methods on the object. You should never use while(true), ...


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