21
I think the biggest problem is the lack of synchronization. You modify the averageTime and count variables in the callback which runs concurrently. You should synchronize the access of this variables. There is a good book on this topic: Read the Java Concurrency in Practice, if you have time read it, it's very useful.
Some other things:
I don't like inner ...
15
First, some smaller things (in terms of the size of code changed):
I think you should use var more, especially when it's clear what type the object has, because you're just creating it.
You shouldn't read the response as a single byte array. Instead, you should use Streams where possible (it certainly is possible in your case), because they are more ...
13
Before I explain how would I improve that code, I'll first mention two problems I will not try to fix (because I'm not sure how to do it properly):
Disposable singleton is a really weird combination. If something should be available anytime and anywhere, it should be a singleton. On the other hand, if something has a limited lifetime and holds an expensive ...
13
I like it,
you could clean up the commented out code and possibly replace
$window.googleMapsInitialized = function () {
mapsDefer.resolve();
};
with
$window.googleMapsInitialized = mapsDefer.resolve;
Personally, I would re-arrange the code a tiny bit, keep the URL completely on top,
keep asyncLoad and the call closer together. Keep ...
13
This is an ugly old pattern. Why don't just try the newer async/await? You already use Task in your tests anyway.
In order to implement it the awaitable way you just need to use a different API, in this case AcceptTcpClientAsync and build everything on top of it. With the CancellationToken you can now better control the server.
public class TcpServer : ...
13
Interesting idea and well-done!
Naming
I really don't like the names. The names like clsMultiThread is somewhat misleading, since as you noted they don't actually provide any true multi-threading. A unwary user would expect it to work with anything and would be disappointed when all of their queued works painfully completes synchronously. ;)
Also, we are ...
10
There are a few items to make this code better.
Naming. Once is an OK name for the class, but the method name IsDone is a problem. This is an 'atomic' operation that sets values, as well as gets values. A method called something like "Trigger", and changing the class name to a common term like OneShot, will give you the semantics like:
private readonly ...
10
Rather than using System Events to generate keystrokes, consider using TextEdit itself to insert text.
tell application "TextEdit"
activate
tell first document to set its text to its text & "Hello World!\n"
end tell
There is a difference, though: this version always appends "Hello World!" to the end of the document, rather than wherever the ...
10
Reflection is slow. That line probably doesn't help your performance :
var model = (T)Activator.CreateInstance(typeof(T));
I notice that you always create your instance with parameterless constructors. So you could add the type constraint new() in your class. That means you could do :
var model = new T();
That'd be faster and well... clearer.
You also ...
10
Quick Review
An API like this, dealing with thread-sensitive operations, requires time and effort to test and review rigorously. When I will find this time, I will do a thorough review. But here are some things I notice right off the bat.
CommMemoryDisposedException should inherit from ObjectDisposedException. This way, consumers can handle your exception ...
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 ...
9
When I have to manage multiple threads, I make sure all processing is complete, typically by using WaitAll.
List<Task> tasks = new List<Task>();
tasks.Add(Task.Run(() => { TriggerAccess(); }));
tasks.Add(Task.Run(() => { TriggerSQL(); }));
Task.WaitAll(tasks.ToArray());
This also allows me to use methods in a non-threaded capacity for ...
9
Sorry to disappoint, but you did everything right! This is a quite good example of using RxUI, Akavache, and Refit together.
The only thing I would change, is to not immediately call LoadItems.ExecuteAsyncTask in the ViewModel constructor. Invoking this in the VM constructor means that your VM class becomes more difficult to test, because you always have ...
8
At a quick glance, there are a couple minor things I noticed regarding how you handle your events:
You are passing null event args. I would instead use EventArgs.Empty, as callers will typically assume the EventArgs object they get from the event handler will be non-null.
You are using Interlocked.Increment on your connection counter, suggesting you are ...
8
I can't comment on how secure your code is, however there are a couple of other problems that I would like to point out.
Clutter
Your using directives can be cut down to:
using System;
using System.Net;
using System.Net.Sockets;
using System.Net.Security;
using System.Security.Cryptography.X509Certificates;
Naming
Let me show you what I think of when I ...
8
Have you considered using HttpClient class from .Net 4.5+? That makes your code lot cleaner.
Also why not take Async pattern all the way? From your initiator (button click or main etc) you can call a async method, which will call all the downstream methods. That way it will be more scalable (as framework will take care of worker threads, scheduling etc)and ...
7
1, Create (at least) two classes. One for UDP and one for TCP. It would omit lots of code like this:
if (this.Type == ServerType.TCP) {
...
} else if (this.Type == ServerType.UDP) {
...
}
Maybe a common abstract parent class and a factory are also required.
2, Anyway, in the following snippet the last else branch should throw an exception:
if (...
7
I made something like this before but I don't claim that it's better.
Although it's much simpler:
public sealed class SingleTask
{
private Task _Task;
private readonly object _Lock = new object();
// You can overload this with TaskCreationOptions, TaskScheduler etc.
public SingleTask() { _Task = Task<object>.FromResult(null); }
//...
7
You are using async/await absolutely correctly, that's how they were supposed to be used.
Please note that from this code it's hard to tell whether the method will return the Task immediately, since it will be waiting synchronously for rasterize method to return the Task, and thus the answer depends on the implementation of rasterize.
As a side note - you ...
7
Assuming that you want to stick with the original plan to insert text wherever the cursor happens to be, you would need something more deterministic than an arbitrary delay.
This script covers all the scenarios that I can think of:
tell application "System Events"
-- In case TextEdit was already running and all windows were closed
repeat ...
7
First, I want to point out the behavior of method that is declared as async void. When the code reaches this line:
var result = await data;
The code following after _service.GetAnimateur is executed. async void is there only for event handlers. So using this signature creates more problems than it solves.
Second, I may not see the whole picture, but your ...
7
I know this is technically outside the scope of the review here, but your first parameter's type is Method. This enum desperately needs to be renamed. When I see Method, I cannot help but think of a programming method... I think you're passing a pointer to a class's method here. This type needs a better, more descriptive name.
Our sender parameter doesn'...
7
If it's really just removing an item from a dictionary (and not e.g. doing synchronous file IO), then Alternative 1 is the best.
When you're using async for scalability (e.g. in ASP.NET), using Task.Run() like this won't help you (since the number of threads used stays the same), it will only hurt you a bit (since Task.Run() has some overhead).
When you're ...
7
A better alternative to the busy wait loop would be to use a Semaphore - in this case a SemaphoreSlim would probably be the most appropriate. Semaphores are not reentrant. This would simplify the implementation to:
struct DumbAsyncMutex
{
private SemaphoreSlim _Semaphore = new SemaphoreSlim(1, 1);
public async Task WithMutexAsync(Func<Task> t)...
7
No that isn't right. If your query throws an error you won't be closing the connection. Swallowing exceptions like that (even though you're logging it) is generally bad as well. How does the user know anything has gone wrong?
If you can, just use async and await with a using:
public async Task DeleteDataAsync(...)
{
using (var connection = ...
7
Using an AsyncTask instead would be much better. You've got many callbacks to the UI thread, all of this you could do in the post execute method instead:
private class LoginTask extends AsyncTask<Void, Void, String>{
private String username;
private String password;
private Exception exception;
private boolean isSaveSuccess;
...
7
General
Your model should not return an ObservableCollection because that collection should be used only if its change notification ability is needed (e.g. with data binding). The method GetDocuments should return an Array or an IEnumerable.
The method InitializeColumns also seems to be GUI related. Consider moving it to the view model.
Background ...
7
From the SendAsync docs:
After calling SendAsync, you must wait for the e-mail transmission to
complete before attempting to send another e-mail message using Send
or SendAsync.
So no, this may not work properly. You need to register an event handler on the SmtpClient.SendCompleted event so that you know the message has sent successfully.
I'm going ...
7
This is way above my expertise, but maybe adding an answer would cause more views/answers? Also, what's that beginner tag doing there? ;)
I want to say first off, really solid work. That's probably why there hasn't been too much activity here. Maybe some of this will seem like nit-picks and if so, Sorry!
ByRef or ByVal arguments
Every one of the arguments,...
7
Ugly typedef
I'm not a big fan of this:
typedef std::future<int> FutureResultInt;
It's not significantly shorter or easier to read, it doesn't isolate the user from an underlying type, and it only serves to slow down my reading every time I hit it. That's somewhat subjective, of course, but I don't believe it adds value. I'd be happier with it if ...
Only top voted, non community-wiki answers of a minimum length are eligible
