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.

# Tag Info

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

6

Dispose Strategy It seems your class doesn't even need to implement IDisposable. Should dispose HttpClient? Single Responsibility I would extract configuration management from the implementation of Client. public Client(string baseUrl) { _baseUrl = baseUrl ?? throw new ArgumentNullException(nameof(baseUrl)); // .. } Add a factory for generating ...

6

It's not bad code for someone new to C++. Here are some observations and suggestions that may help you improve your code. Avoid relative paths in #includes Generally it's better to omit relative path names from #include files and instead point the compiler to the appropriate location. So instead of this: #include "Server/Routes.h" #include "Tools/Logger....

5

Seems a bit needlessly complicated and not exactly async at all. You're just forcing an async call to be synchronous with .Wait(). Proper way is to use async "all the way down": [HttpPost] [Route("api/v1/upload")] public async Task<HttpResponseMessage> Post([FromUri]string filename) { try { using (Stream requestStream = await this....

4

Since you are not showing a whole lot of the class itself, there is not to much fundamental to be done on the presented code. Documentation First, you should add documentation to your class and to its methods. Use the """...""" to write those docstrings as recommended in the official Style Guide. Style The Style Guide also has a section on variable ...

4

I think, you should be concerned about this behavior: public class Client : IDisposable { private static HttpClient _client; public Client(bool payment) { var baseUrl = payment ? "https://payment.apiaddress.com/" : "https://api.apiaddress.com/"; _client = new HttpClient {BaseAddress = new Uri(baseUrl)}; _client.DefaultRequestHeaders....

3

When programming in C or C++, there are really many ways to shoot yourself in the foot. To prevent at least some of them, the compilers can produce warnings if they see typical mistakes. But you, as the programmer, have to tell the compiler to give you these warnings. For GCC, the basic warning level is -Wall. It issues many useful warnings. So instead of ...

3

(I only looked at the GitHub repository ...) Ah you've got go vet already on, that's why it's not finding anything, great! Dockerfile looks good too. You might want to specify an exact Go version just in case. main.go looks fine to me. Only the unused return value from router.Run() would pop up depending on your linter settings, which you could ...

3

The Java issues are already described above, so let me add Eclipse-specific points. This code will handle exceptions as expected in the main thread only (and will throw InvalidThreadAccess otherwise). But it is not a good idea to work with I/O in the main thread as the user will be unhappy with a frozen UI. Showing dialogues from some operation is bad ...

3

When running your program with the Go race detector, it found no race conditions. That's good! Given this, I would consider your "execution scheduling" valid. Is it appropriate to use defined-size channels here? Sure. In this case you make the buffer large enough such that it will not be blocked. You can read more about the difference between buffered ...

3

Full code import (...) Please include your full code next time. This should instead be: import ( "database/sql" "errors" "fmt" "io/ioutil" "log" "net/http" "os" "path/filepath" "sync" "time" ) You also have item.toString(), but this is nowhere defined. I changed it to: fmt.Println(item) Scope connectionString ...

3

Try to avoid using magic values. This one is a hack to avoid writing 2 methods. private HttpResponseMessage TryCatch(Action action) { return TryCatch(() => { action(); return "ActionToFunc"; }); } You are better of splitting your methods. Dispatch error handling to its own method ResolveStatusCode to avoid boiler-plate error handling. In addition, ...

3

Let me add a few quick comments to the other review(s). In general, pay attention to const correctness to reduce the chances of making mistakes and also to avoid unnecessary - and potentially expensive - copies. You have plenty of "getters" of the form get_... (e.g. get_version(), and so on) that don't modify the state of the object. Write these as ...

2

Initialization in Constructors Currently the constructors perform the initialization within the body of the constructor, this isn't necessary for simple variables, C++ has a shorthand form for initializing variables HTTPRequest::HTTPRequest(std::string _hostname) : hostname{_hostname}, method{"GET"}, path{"/"} { set_args(); set_default_headers(); } ...

2

A couple of generic good practices are Try to avoid using global variables(You can pass wg, ch & errItems as arguments to function) Single Responsibility function(A function should do only once task. Makes it easy to test.) As far as possible pass dependencies as variables or using dependency injection etc. Specific to this snippet you can: Pass ...

2

I don't program in C and I don't know the exact input strings that you're expecting, but I can offer a couple of regex suggestions based on reasonable assumptions. Your regex: @"Host: (((?<hostName>.+?):(?<port>\d+?))|(?<hostName>.+?))\s+" Your pattern looks for a hostname followed by a colon then a port number then whitespace characters ...

2

Because you specifically ask about security: For comparing hashes like SHA, you should use hash_equals(). If you are requiring the use of a password, you shouldn't use SHA-512. While it's certainly better than MD5, instead use bcrypt. Bcrypt is heavily recommended for password storage, especially long-term. With bcrypt, instead of using hash() and ...

2

I want to start off by saying dfhwze is the correct answer. I just wanted to touch more on the magic string "ActionToFunc". Sometimes, especially with functional programming, you need to convert an Action to a Func<>. I would copy what is already proven and working by borrowing from F# and Rx and create a Unit struct Here is documentation on F# ...

2

Well, I didn't take the time to test my snippets, but here are two suggestions... #1 (preferred) - Bundle all of your input data into a single array, then pass it to your receiving url with a single curl call. It is best practice to minimize total calls (avoid iterated calls) so long as the operation works for your project, of course. This avoids sleepy ...

2

Elaborating on RoToRa's comments: If you are going to build a string by concatenation, then using a string builder like that only improves performance if there is a loop. That doesn't apply here. So your code could be written as: String logString = "Request URL: ") + uri + " Query String: " + queryString + " Session Id: " + httpRequest....

2

The redirect doesn't seem to be part of the logging. Maybe put it in a separate filter that runs before this one? The logged string could be built using String.format() or MessageFormat which would be more readable. Most logging frameworks have such formatting built into their logging methods anyway. Do you really need to convert the post parameters into ...

2

def bigcommerce_api_v3_get_customers_by_shop(shop, options = {}) Since you are only extracting a couple values from options here, consider using keyword params instead. It makes it more obvious what specific keys are used by the method, without needing to dig into the method body. raise if params['page'] || params['limit'] # prevent manual pagination ...

1

Declaring a class makes sense if it can be reused, but less so if the logic and element ids are hardcoded to just one case, it only adds the unnecessary this. to variables. It's unclear why the click listener is handled outside of the form class as it's logically and conceptually a part of the form. JS naming convention is camelCase, not snake_case. Consider ...

1

Not too much to review as the code uses evhttp to handle HTTP and provide a basic, static reply for each request. Why is host constexpr, but not the port? All those using statements, combined with traits and decltype one after the other makes the code very hard to read, almost obscuring the fact that evhttp is being initialized at that point. I would try ...

1

There are few things that can be improved, will start with the major ones: General Instead of having function for each API call you can generalise it a bit like so: /* Updated the name, since it's not only for transactions anymore Since we no longer know the exact endpoint we will have to pass it to the function I would also add requestMeta parameter ...

1

Some suggestions: if __name__ == '__main__': is usually followed by a call to a main method. The main method then parses arguments using for example argparse to inject all the things which should not be hardcoded in the application - definitely passwords, but in your case probably all of the parameters to WebMarketingContent. This makes the code reusable ...

1

Error Handling Your error handling is pretty inconsistent. Sometimes you just unwrap: poll.poll(&mut events, Some(to)).unwrap(); Sometimes you use expect: call.perform(htp, &poll).expect("Call failed") Sometimes you use ?: let poll = Poll::new()?; Sometimes you match: match do_call(&mut htp, &poll, call) { Ok(...

1

The most critical part is that this code is not a class. It's a function. A function just written in a form of a class method but it's a function all the same. Ok, it is using distinct methods to get the response and the code, but that's not enough to become a class. For this purpose you can make a function to return an array, like this return ['response' =&...

1

Use class constants to store values that will remain the same and are unchangeable Your HTTP method verbs are better off set as class constants than class properties since they will remain the same and are unchangeable. (i.e. constant) private $POST = 'POST'; private$PUT = 'PUT'; private $GET = 'GET'; private$DELETE = 'DELETE'; private \$PATCH = 'PATCH'; ...

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