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 ...
6
I'd still say that you should break apart the mail message and SMTP client into separate classes for the "Single responsibility principle". In specific:
A class should only have a single responsibility, that is, only changes to one part of the software's specification should be able to affect the specification of the class.
This is usually where you have ...
5
I'm trying to balance the code readability/size with the performance
benefits, ..
Your code could be optimized for readability and object-oriented design without introducing a performance penalty.
The entropy generation could be written more elegantly.
original code
for (char a = '0'; a <= '9'; a++) {
for (char b = '0'; b <= '9'; b++) {
...
5
Stylistically, the for loop is more readable. As a general rule, if you start the loop at 1, then I would prefer the termination condition to use <=; if you start counting from 0, then < would be better.
The design of this function is conceptually flawed. It will return an unused filename, but presumably you will eventually want to create a file ...
5
GroupBy vs ToLookup
From reference source: Linq Enumerable
Dictionary<object, HashSet<T>>> can be replaced with a ILookup<object, T>.
public static ILookup<TKey, TSource> ToLookup<TSource, TKey>(
this IEnumerable<TSource> source, Func<TSource, TKey> keySelector)
{
// impl ..
}
There is also an ...
5
public static IDictionary<string, Dictionary<object, HashSet<T>>> MultiGroupBy<T>(this IEnumerable<T> source, params (string Label, Func<T, object> Getter)[] groupers)
I don't understand the mixture of interfaces (IDictionary) and implementations (Dictionary, HashSet), nor the mixture of generics (<T>) and non-...
4
If you only want to enumerate source once, then you'll have to cache it somehow. Either by materializing it immediately, as you do, or whenever the first group is enumerated, but that's more complicated.
If you don't mind duplicate entries in your groups and throwing on duplicate labels, then your code can be simplified to the following:
public static ...
3
A little late answer. I have focused on CommMemory and tried to refactor it as I see fit. You have discussed most of the issues with dfhwze, so I have not much to add other than what I have commented inline in the code below. You may agree or not in my changes and/or use it as you want. Any way it was fun to review.
I have removed most of your own comments ...
3
return "page.aspx?id={id}&ref={ref}".Replace("{id}", id)
.Replace("{referrer}", referrer);
The above can be written since C# 6 and ECMAScript 6 as..
C#
return $"page.aspx?id={id}&ref={referrer}";
Javascript
return `page.aspx?id=${id}&ref=${referrer}`;
3
Specification Considerations
Note that the first caller that gets access is the one that provides the cancellationToken for the valueFactory. Other callers use their own cancellationToken for timing out on awaiting the result of the async operation. This is how you designed your class. I did not change this part of the behavior.
If this behavior is not ...
3
As others have pointed out, the code should be object oriented. It’s OK to start with procedural code as you have, but if you do, you should get in the habit of writing a quick test in your main entry point. Once you’ve written the first test, it can help you start seeing objects more clearly (and drive you to further unit tests.)
For example: an ...
3
Review
GetTimeSlotForAll does 2 distinct things: (1) it loads time slots by people (2) it slices time slots to get shared free time. Each of these operations should have its own method. (single responsibility principle)
GetTimeSlotForAll should return IEnumerable<TimePeriod> rather than List<TimePeriod> because it's intended to enumerate upon, ...
2
Review
Don't use regions to group members by type. This is redundant grouping. (Regions pattern or anti-pattern?)
Use proper naming conventions and casing of variables. LockSync is generally called syncRoot. GetNewTokenSemaphore indicates a method name, rename it to newTokenMutex. It's a mutex because you use the semaphore as a mutex.
Prefer TryGetValue ...
2
Personally I tend to avoid creating classes, which do not implement any logic. Sometimes it makes sense or is even necessary, but always when I see such a class, I try to find out what operations are performed on its data and if they shouldn't be implemented as methods of that class.
What I don't like about your code is the ProcessCart method, which pretty ...
2
Major Issues
The class access modifier does not allow public access, so your class is not usable by third party consumers. Make it public.
Your class should be generic to be usable.
You should always provide unit tests when providing a class to consumers of your API.
Pop is ill-defined. It does not return anything. It can throw a NullReferenceException when ...
2
I'd make the following suggestions:
avoid placing limits (e.g., pre-declaring the tape size) if you can;
use properties and small functions to abstract away the lowest-level details;
use conditional expressions (cond ? y : n) in preference to if-then-else blocks;
traditionally, each instruction (state) in a Turing machine specifies, for each possible tape ...
2
Review
hasTerminated should be a public property with private setter and be renamed Terminated.
TuringMachine could be a sealed class so you don't need to worry about which private data should actually be protected and which methods virtual.
List<ControlCard> cards should be IEnumerable<ControlCard> since you do not add/remove elements from it.
...
2
I'd like to advocate for a more functional programming style. We all know object orientation can provide clarity over procedural programming, but if you can replace for loops and if statements with Select and Where? I'd say that's even better.
Here's how:
public static int GetMoneySpent(int budget, int[] keyboards, int[] drives)
{
var ...
2
In terms of the times on LeetCode, they can be dependent upon the server load as much as the code. I tried your code and got a range of times from 100 ms to 136 ms. Holding onto the last node is a better solution than repeatedly trying to find it but on a list of only a few nodes I don't know how much impact it would have.
In terms of the code, this ...
2
You are disposing the message before it even has a chance to be used by a consumer.
Remove the using block in the factory method
public override MailMessage CreateMailMessage(IEmailNotification emailNotification) {
var mailMessage = new MailMessage();
mailMessage.From = new MailAddress(emailNotification.From);
mailMessage.To.Add(...
2
I have some tips regarding C# conventions.
Public fields should be replaced with public properties and should be title-cased. For insance Board.width, Board.height.
Think about whether certain fields should be encapsulated within the instance and can only be mutated by methods on the instance. Tile.tileX, Tile.tileY are good candidates to encapsulate to ...
2
dfhwze has addressed the structural and other issues with the code, so I will focus on eliminating the pyramid of doom in AllPasswordCodes:
private static IEnumerable<byte[]> AllPasswordCodes {
get {
for (char a = '0'; a <= '9'; a++) {
for (char b = '0'; b <= '9'; b++) {
for (char c = '...
2
FYI. I've now tested the different versions of the algorithm from the different answers, and the result is as follows:
Data Size: 10
Name Iterations Average Min Max Total Std Dev Units
Pieter Wit: 50 0.38341 0.05530 16.09750 19.17070 2.24480 [Milliseconds]
...
2
Consistency with LINQ
I find in order to be consistent with other LINQ APIs and to make the usage of this extension more intuitive you should slightly adjust the parameter names and the return value and rename it to ToLookups.
ToLookup calls the Func keySelector and since this extension is accepting a collection, I suggest the name keySelectors.
As far as ...
2
You probably want to
add a constant for speed 0
create a test for MaxSpeed
create a test case for 64 (it's one edge of the partition class 0 <= x < speedlimit and it could replace 60 which is in the same partition class)
I would keep the 66 test case since it is and edge case.
Even though the edges are cut pretty clear with ints (compared to using ...
2
Am I using too many test cases for CalculateDemeritPoints_WhenCalled_ReturnsExpectedInt?
Yes. I'd say so, anyway.
Let's say I'm maintaining code you've written. When a test case fails, ideally it will give me exactly the information I need to fix it, and it will give me that information as quickly as possible. The very first thing I will see when I learn a ...
1
Since this code is a black box, because it calculates everything itself without calling any dependencies the caller can be aware of, we should test this method with sufficient variations of input to yield us:
any speed < 0 should yield error
any speed > 300 should yield error
any speed between 0 and 65 (bounds included) should yield 0
any speed ...
1
You could simply add the task results to a collection. Different types of collections can be used:
List<ITaskResult>: You can add several results of the same type. You must enumerate to find a task of a specific type.
Dictionary<Type, ITaskResult>: Each task result type can be added only once. You can query specific task types.
if (results....
1
It appears that the string you pass into the method is a type of picture not really a filename, since the filename needs an index attached.
I'm not a big fan of either approach. I think using GetFiles and using the Length property is easier to see what's happening.
Something like this should work:
using System;
using System.IO;
private static string ...
1
Comparison
A comparison of performance of 2 classes only makes sense if both adhere to the same specification. Does your class do what a ConcurrentDictionary does?
Review
Why would you allow access to the underlying dictionary? If you must allow it, return a IReadOnlyDictionary.
Checking arguments before taking a lock prevents unnecessary locks on bad ...
Only top voted, non community-wiki answers of a minimum length are eligible
