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
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)...
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
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
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
Your code is absolutely correct in case when you want to start workflows only when all of them are configured.
But if you want to start each workflow once it's configured (independently from other workflows) then it might be a good idea to use continuations... In .NET 4.5 it would look like this:
public sealed class Engine : IEngine
{
private readonly ...
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
At first I tried to make it completely lock-less, but it turned out to be impossible. This implementation contains a lock however the lock isn't an instance wide.
It may shock you to hear that lock-free code may be slower than code using locking.
Lock-free code is usually longer and is definitely much more complex. You should not attempt to write lock-free ...
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
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 ...
7
I don't think that screen space is so limited that methods can't be separated by a blank line, and separation makes it slightly easier to see scope.
public delegate void NotifyCBR();
private NotifyCBR notifyCbr_ = null;
Is there any reason not to use System.Action?
private System.Collections.Generic.List<T> vector_;
...
Only top voted, non community-wiki answers of a minimum length are eligible
