New answers tagged

0

Task Parallel Library (TPL) I believe you are reinventing the wheel here. TPL provides numerous ways of synchronizing tasks. For instance, it allows you to: create a task scheduler from current synchronization context or to start a task on a specified task scheduler. All you need to do is create a custom synchronization context (.NET Core no longer ...


2

There are not much to add to what have already been said about coding style and conventions. A little optimization on the input check could be that if head.next == null then you can leave early as well: if (head == null || head.next == null || k <= 0) return head; Now it's known that there are at least two elements in the list, so calculating the ...


2

You have difficulties getting rid of the repeated code because you're trying to force two different operations into one: parsing html converting the result into OLE color Start with extracting the default value and make it a field: static readonly Color DefaultColor = Color.FromArgb(224, 224, 224); Then create the main API and give it a name that clearly ...


2

Another option, though not without caveats, is to reverse the logic. Something like this should work: private int GetOleFromHTML(string stringRep) { Color c = Color.FromArgb(224, 224, 224); if (!string.IsNullOrWhiteSpace(stringRep)) { try { c = ColorTranslator.FromHtml(stringRep); } catch { ...


2

Since a method has to be executed to get this color, you cannot declare the color as constant. A static readonly field is what comes closest to a const. private static readonly Color DefaultColor = Color.FromArgb(224, 224, 224); private int GetOleFromHTML(string stringRep) { Color c; if (string.IsNullOrWhiteSpace(stringRep)) { c = ...


1

Review Note that I'm using arrow notation which was not available back then. But that does not impact the issues addresses in this review. Method overload resolution allows an explicitly implemented interface method to call the implicitly one: IEnumerator IEnumerable.GetEnumerator() { return new NListEnumerator(this); } IEnumerator IEnumerable....


1

The variable ex is not used anywhere else, so it can be in-lined. string strLogText = FormatException(context.Exception); The multiple calls to DateTime.Now will give different time stamps. Hold on to the timestamp in one variable early in the function and reuse that. It was already assigned here var timeUtc = DateTime.Now; so use that //... ...


3

I see the point of the commenters: why implement this, if it has already been implemented? However, I still see benefit for such a question in an interview process. Given the right analysis, you can see how the developer thinks and what question he asks, which is IMHO more important than the actual code. Questions could be: What coding guidelines shall I ...


1

ListChangedType.Reset AddItems is nothing more than a glorified wrapper method for calling Add multiple times. However, guidelines suggest that ListChangedType.Reset could and probably should also be called here. Since Reset should be called when.. Much of the list has changed. Any listening controls should refresh all their data from the list. To ...


3

Performance optimization int listLength = 0; while (tail != null) { listLength += 1; tail = tail.next; } Since head != null we can start listLength at 1 and keep our tail by checking tail.next != null rather than tail != null. int listLength = 1; while (tail.next != null) { listLength += 1; tail = tail.next; } Now we no longer have to ...


6

Using an uppercase name for a variable is a bad practise; should be rotatedList or preferably res for result or something similar. Separate your instantiations with semicolons: ListNode tail = head; ListNode rotatedList = null; ListNode kthnode = head; ListNode kthPrevNode = head; Your naming of head and tail ...


7

if (k > 0 && head != null) { ... return RotatedList; } return head; would be clearer (and more consistent with the other special case) as if (k <= 0 || head == null) { return head; } ... return RotatedList; (although really if k < 0 I think it should ...


3

Since I couldn't choose a single answer as the answer, I decided to compilte all the suggestions and comments made and show how the final class(es) looks. It still doesn't use ReaderWriterLockSlim, but the code is far cleaner and contains one or two less bugs. Maybe the final results can be used by others :) thanks to everyone for the feedback, I love this ...


3

SolverOptions public class SolverOptions { public int MaxRecursion { get; set; } = -1; public int MaxSolutions { get; set; } = -1; Instead of using undocumented magic values, why not use uint?? I'm not sure what MaxRecursion gains you. IMO it would be more useful to have a progress report and a way to cancel the search. public bool ...


6

Cosmetics You can simplify your code by removing the generic arguments from LRUCacheEntry. Since it's a nested class it can use its parent class' types without redefining them. You could also make it a readonly struct and save a couple of instances. private readonly struct LRUCacheEntry { public readonly TKey Key; public readonly TValue Value; ...


8

This is a neat little implementation. It's nice that LruCacheEntry is a private inner class. It does indeed look to be thread-safe in its current state. When calling Add, if the key is already present, then a new node is added to LruList before the code crashes because the key is already present in the dictionary. This means you could end up with an ever ...


10

..but I expect that most 4/5 cache accesses will result in a hit, and 1/5 in a miss followed by an add. You are not exposing your sync root _lock, meaning this class isn't thread-safe for consumers. The only meaningful way to use your class is as follows: if (!cache.TryGet(key, out var value)) { // watch out! one of the other threads may already have ...


7

/// <remarks> /// Providing the value 0 to <paramref name="capacity"/> /// effectively disables the cache. /// </remarks> _capacity is a private readonly field. I can't think of a situation where to instantiate an cache object with an immutable capacity set to 0 - hence useless, but you may have some idea with that? lock (_lock) { ...


0

In my opinion, our class name should be like " which clearly speaks the purpose of the existence. E.G. : When we are creating a entity class just to bind it in the UI layer, it make sense to call it with suffix like CustomerModel. Also if you are creating a class in the business layer or Data layer where it will be communicating to database, you can name it ...


7

Given that the PdfWriter holds a reference to an IDisposable resource (the Stream), it's good practice to implement IDisposable. Note that Stream is a special case of rule CA2213. I've also noticed that all of your examples call both Initialise and Finish. If that's the case, I'd argue for a factory method to create the writer and call Finish within the ...


0

Remove static modifier from IWebDriver instances, my expectation is that at some point you will need to run your tests in multi-threaded manner to speed up execution or perform cross-browser testing and if the IWebDriver will be static - it will be shared between different threads causing race conditions Default section is missing from your switch statement ...


2

(self-answer) This thing has grown a little bit since I have posted it. I followed most of the suggestions and added some new features: dynamic parameter resolution for middleware constructor and for the Invoke methods. I now throw exceptions when contract conventions have not been met like invalid middleware constructor, too few or too many invokes or ...


4

The code in question is using the wrong tool (Regex) to achieve what you want. Using the string representation of a Color to get a string back is not a good way. If one would pass a predefined color like Color.Red into that method it would fail by returning R = , G = , B = because the Color.ToString() returns A string that is the name of this Color, ...


2

Developer gone 'rogue' I am not entirely happy with this code. The reason is, in the future, a developer may forget that HtmlSanitizerFactory should be used for instantiating HtmlSanitizer... so he would create his new ModelBinder .. For this to happen, a developer has to dive in your code and deliberately change private HtmlSanitizer _htmlSanitizer ...


5

I have some style and organization comments to add to @dfhwze 's previous answer about performance and style. For ListNode class, I would expect val and next to be named Value and Next. I would also rather see them be properties instead of fields. And for some reason, I am expecting to see a Previous property as well. I see nothing in the exercise ...


3

After running through a few ideas in my first pass at the current implementation, I ended up with this refactored approach to simplify your build process public RequestCallback<TContext> Build<TContext>() { var next = new RequestCallback<TContext>(context => Task.CompletedTask); while (_middlewareTypes.Any()) { var ...


5

internal class Client : IDisposable { private static HttpClient _client; private static Uri _baseAddress; Neither of those fields should be static. private static Client _paymentClient; private static Client _mainClient; public static Client Create(bool payment, Config config = null) { if (!payment) { ...


6

Performance You have implemented Floyd’s Cycle-Finding Algorithm which adheres to \$0(1)\$ storage space. An alternative exists Brent’s Cycle Detection Algorithm which uses the same storage space. Check out this review on Computer Science SE for a comparison. It appears in general, Brent's algorithm is faster. According to Brent's paper, the complexity ...


2

Scheduling & Latency One single handler could block all other handlers by performing a long-running calculation: public void Handle(SavingChangesEvent message) { Thread.Sleep(10000); // long running code .. } While this is the exact same behavior of the default Event Pattern in C#, you might expect an aggregator to be able to work around this - ...


2

That is an elegant implementation using the hyperoperation. The disadvantage is that a branch on the “level” is needed in each recursive step. I would implement the addition, multiplication and exponentiation as separate functions. That makes the level parameter obsolete. It is more code but (in my opinion) much clearer: public static int Evaluate(int left,...


2

Conventions If you want similar behavior as ASP.NET Core, you should check against both methods Invoke and InvokeAsync. If not, I would prefer to change the order to seek the methods. Postfix *Async is a convention to return Task. View component '...' must have exactly one public method named 'InvokeAsync' or 'Invoke'. Dependencies You have included ...


6

Review I see no reason to have 3 constructors, each invoking the other, when two of them are private and only called by another constructor. Keep it simple. You check for if (Nodes == null) in the constructor. Is there a scenario where this reference could have already been created upfront? Constructor arguments should be camel cased. There is no point in ...


9

If you modify the AsyncDictionary while enumerating its keys/values it throws InvalidOperationException (if the backing dictionary is a Dictionary). var numbers = new AsyncDictionary<int, int>(); foreach(var number in Enumerable.Range(1, 1000)) { await numbers.AddAsync(number, number); } foreach(var number in await numbers.GetKeysAsync()) { ...


11

It's pretty hard to break something that uses a global lock around everything. So this seems pretty thread-safe. But that doesn't answer the question of why you'd want to use this. Asynchronous calls are useful for things that take a long time, particularly if you can delegate the "waiting" to some low-level event based solution (HTTP requests for example)....


-2

I am doing a similar thing, but I created an IParamEvent. I then create a class for each event and inherit from IParamEvent. public interface IEventParam { string Name { get; set; } } class CarSpeedParam : IEventParam { public string Name { get; set; } public int Speed { get; set; } }


24

The reason why there is no async API for a dictionary is, that all operations on a dictionary are so fast, that there is no need for asynchronicity. For concurrent scenarios there is the thread safe variant - the ConcurrentDictionary. Adding an async API to these dictionaries has absolutely zero value. Rather it increases complexity and reduces performance....


11

Threading Design Your implementation has a very intrusive lock for all read and write operations, using the SemaphoreSlim with max concurrency 1. try { await _semaphoreSlim.WaitAsync()// <- both read/write operations acquire single mutex return await Task.Run(async () => { return await func(_dictionary, keyValuePair); }); } ...


4

public enum Operation { Addition, Multiplication, Exponentation } If you gave these explicit values: public enum Operation { Addition = 1, Multiplication = 2, Exponentation = 3 } ... you could avoid the + 1 for the operation: return Hyper(left, (int)operation, right); Because Hyper(...) is private, and you are supposed to ...


2

Cancellation Token Source You create a token source, and then immediately overwrite it with a new instance, disregarding the previously created instance. Surely, you did not mean to instantiate it twice? _runningSubscriberCancellationTokenSource = new CancellationTokenSource(); _runningSubscriberCancellationTokenSource = CancellationTokenSource....


5

As dfhwze says, you should probably pull in the connection string/information from an external source: the production database hopefully won't be called 'TEST', which means you can't use this code in production, which means you will be adding time, effort, and the opportunity for things to go wrong to deployment. Similarly for the web API URL: it might ...


5

Review Single responsibility principle Method save_vendor_info (which we'll rename later) does data retrieval from an external source, xml to db mapping, and SQL command execution on a database. I would expect a method with the name Save* to only store data, in this case in the database. I would also suggest to split up loading the input data and saving to ...


4

Review Whitespace Use whitespace between the method name and opening parenthesis: Main (string[] args) instead of Main(string[] args). The same convention applies to statements such as while: while (Continue) instead of while(Continue). And also to curly braces: new string[4] { instead of new string[4]{. New Line Prefer using the new line of the system ...


3

You solution look fine so far (maybe to much empty lines, but that's just peanuts ;)). If you are intested in a more object oriented approach, I'll show you an alternative solutions. It looks (and is) quite overengineert for such a simple problem - but for real problems it is often a good choice :). First, abstract the 4 mathematical operations and it's ...


1

If you need performance optimized code, your solution looks very well. I think the performance can be optimized best by using a simplified aproximation approach (e.g.: Math.Abs(value1 - value2) <= Double.Epsilon). Another note (that actually doesn't affect performance): Use '||' instead of use '|' because the second one evaluates always both conditions ...


3

SOLVED this is the way I ended up doing it, don't know if it's the best way, but it works I took this idea from an answer that was posted here but now it's not anymore, and Honestly I don't remember who suggested it, I took the foreach loop and adapted to my case, for my point of view the foreach loop is the fasted way, and also I can turn it into a ...


3

Compactness use a switch statement rather than verbose if-elseif-.. statements get rid of redundant blank lines Readability Avoid escaping characters. Console.WriteLine("Type \"+\" for addition"); Console.WriteLine("Type \"-\" for subtraction"); Console.WriteLine("Type \"*\" for multiplication"); Console.WriteLine("Type \"/\" for division"); You could ...


3

Wait, are the arrays already sorted? These are just my initial thoughts, but: The initial creation of your new list I'm guessing is \$O(n)\$ to create a copy of array a (\$n\$ is the length of a). This might be unavoidable if you want a nondestructive function, though the arrays are passed in by value, so it may not be necessary. List.Exists is that it's a ...


1

Error Handling You are relying on OAuthGetRequestTokenAsync to catch all exceptions for you. flickr.OAuthGetRequestTokenAsync(callbackUrl, r => { if (r.HasError) tcs.TrySetException(r.Error); else tcs.TrySetResult(r.Result); }); Unhandled errors However, OAuthGetRequestTokenAsync internally calls FlickrResponder....


0

I can only give you some general tips; I'm afraid Unity is outside my expertise. I would consider redefining what renderDistance means. Currently it seems like a diameter; the rendered terrain consists of renderDistance2 chunks. What that means in practice is that every time you reference renderDistance, you divide it by 2. What happens if it is odd? It may ...


1

Race Condition You are starting two tasks concurrently, each blocking on an event to occur. var res = await Task.WhenAny(ReadAsync(stream, buffer, ct), GetPayload(ct)); By calling WhenAny, once one of tasks is completed, you continue without awaiting completion of the other task. Let's say that ReadAsync completes first, this means the following code rus ...


Top 50 recent answers are included