New answers tagged

2

Just a few remarks. Extensionmethods. AndClauses() You should remove the else which saves one level of indentation. The code itself. That code would benefit as well by reverting the if condition to return early. This saves one level of indentation as well. I would change memberChain.Any() to memberChain.Count > 0. There is no need to use an enumerator ...


2

Quick remarks: Do not call things "xxxxList". The type might change and thus the name might become invalid, but more importantly we don't call a collection of items an "xxxxList" in real life, we simply use the plural: areas, requirements, etc. Use descriptive names: sortedList tells me next to nothing about the contents of that ...


2

ExpressionParser is too general, you might need to renamed it to be specific such as MathExpressionParser. ExpressionParser should be abstract class along with Variables property. Computer in ExpressionParser can be declared globally, and used across the class. in the Compute method, you're not validating the string, you should add some validations such ...


2

var when appropriate These lines: List<Requirement> areaPathOrderedList = new List<Requirement>(); List<string> sectionHeadingList = new List<string>(); have an obvious type, so you can replace them with var. For your other variables, keep them as-is since their type is not obvious on inspection. C-style loops ...


5

I can see three small improvement areas: Use ctor instead of object initializer Instead of this: new DataColumn(v.Key) { DataType = v.Value.GetType() } Use this: new DataColumn(v.Key, v.Value.GetType()) Object initializer will run after ctor. If ctor accepts that parameter then prefer to provide it in the ctor. Use early exist instead of guard expression ...


3

You can define a mapping between Types and the Deserializers. You can do this for example like this: public class GenericDeserializer<T> : IDeserializer<T> { private readonly ImmutableDictionary<Type, object> _deserializers = new Dictionary<Type, object> { { typeof(double), Deserializers.Double }, { ...


0

You can get rid of the branching in the Deserialize method and instead move that logic to a place that is executed only once - the construction place. An instance of your GenericDeserializer<T> will always execute the same branch for any input, because the instance is already tied to a specific output type T. For the types like int, and double where ...


1

I think you can achieve the same with less code by utilizing some libraries and language features. Google's Sitemap Protocol is not supported by default in the .NET Framework / Core. But there are couple of packages which defines the corresponding C# classes, for example: Google-Sitemap X.Web.Sitemap I have chosen the second one. So, the parsing logic ...


0

Thank you all for your input. I was able to dramatically make it more efficient by creating models and using CSVhelper. https://joshclose.github.io/CsvHelper/ to export all the historical records to disk, only once during transitioning to realtime. Much faster. var trendchars = new[] // array of arrays { input1_1_trendchar, input1_2_trendchar, ...


2

My try to prettify the code. Some comments inside. private void WriteToCSV2() { if (CurrentBar <= BarsRequiredToPlot + LookBack) return; StringBuilder logEntry = new StringBuilder(); string fullPath = Path.Combine(filePath, fileName); // don't need a Stringbuilder to build a constant string const string header = "Index Key,...


4

First I would like to re-iterate the points made by @cliesens regarding naming and avoid uint. Keep in mind the most number of elements you may have in an array is int.MaxValue. Theoretically, with your use of unit, someone could try to create an array that is 3 billion wide by 3 billion deep, which would throw exceptions. And your code doesn't handle ...


0

I think there is plenty of room for improvement. Whenever I write code I try to focus on 3 things in this order: Does the code correctly perform its purpose? If another developer reads this code in 6 months, will they understand it? Does the code perform optimally? I think you fall short of (2). The thing that slaps me in the face is why is a List used ...


1

Trying to not promote low-level transaction implementation in initial design, IDbContext is the place for db-transactions. public interface IDbContext { void Commit(); void Rollback(); } Using “Generics” to implement “Unit Of Work” public interface IUnitOfWork<T> where T: IDbContext { T DbContext { get; } } Now the async ...


0

This is pretty abstract and might be a better fit over in Software Engineering or Stack Overflow. I'm going to leave out if this is a good idea. I don't use repos hardly anymore but only you know if that's required. I think for simple projects they are fine but there are better patterns for more complex projects, IMO. I don't like the generic as there is ...


3

Not a review but my version of the same as usage example of Task, BlockingCollection and IDisposable implementation. It's thread-safe, thus you may create it and add jobs from multiple threads. public class Consumer<T> : IDisposable { private readonly int _maxConcurrency; private readonly Action<T> _action; private readonly ...


5

Welcome to Code Review! I don't have much time to write a full review right now, but I'll give you a concise list of things you could do to improve your code (in my opinion). Naming Use camelCase instead of PascalCase for local variables, class variables & method arguments. In addition to making your code a lot more readable (I don't have to wonder, &...


4

This: Console.WriteLine("Index invalid"); return default(T); is dangerous. Having out-of-bounds indices is dangerous enough that you should probably just raise an exception here, rather than returning a default that will probably be wrong from the perspective of the caller. If this is a feature that you're really going to use, wrap it in a ...


3

Re-entrance I don't think that these: private static TinyIoCContainer _container; public static Double ScreenHeight { get; set; } public static Double ScreenWidth { get; set; } should be static. Particularly for the purposes of unit testing, these should simply be instance properties on your App. The feasibility of this might be ...


0

I have some doubts about the way you choose to implement "unit of work". By definition, this pattern is an abstraction between the database access layer and business logic. Which means that the "unit of work" should not have database code. Having database-specific implementation of the "unit of work" is an indication of wrong ...


11

Some Notes : you should make your code more readable more often. This would give your code more sense even for yourself, specially if you reviewed it in the future. you need to consider naming convention like using Pascal Casing for Properties, naming your variables with meaningful names. Don't split the string without validating first. Don't Parse values ...


0

I would advice starting with writing code that is actually readable. Also from your question I don't really know what your goal is. Do you want short code that get the job done? Then I'd say you are already there. If you want readable and maintainable code, then maybe try an OO approach where you encapsulate bits of code into objects which with their name ...


3

You have entered in the realm of concurrency. So there will be one or more functions which are competing to be executed. In other words there is no guarantee that your code will run in parallel. If there is enough executor (CPU cores) then the runtime might schedule several threads to different cores. If there is not sufficient resource to be utilized then ...


0

public static void GenerateSequence() { double tenPower(double n) => Math.Pow(10, Math.Floor(Math.Log10(n)+1)); double build(double n) => n * tenPower(n + 1) + n + 1; double merge(double n1, double n2) => n1 + n2 / tenPower(n2); var n = 1; var ans = new double[16]; for (int j1 = 0; j1 < 16; ++j1) { n += j1; ...


0

What if you nested the MemberExpressionVisitor class inside another class? Since I like extension methods, create a static class that contains an extension method on Expression<Func<T,TRes>> that creates the visitor, runs it and returns the result: static class LambdaExt { public static List<string> GetAccessedMemberNames<T, TRes>(...


2

The code pattern var x = new List<X>() followed by a foreach adding to that list is always a telltale that LINQ will probably make things easier and more comprehensible. Your code is not exception to that rule. I'll show you how this can be done quite concisely using LINQ and then explain a few things: var groupedSubscriptions = subscriptions.GroupBy(...


0

DRY = Don't Repeat Yourself. If you have repeating parts of code, something went wrong then. Also you may use Linq. Consider this example. static void Main(string[] args) { string[] lines = File.ReadAllLines(@"C:\temp\andmed2"); List<int> numbersList = lines.Select(line => int.Parse(line)).OrderByDescending(x => x).ToList(); ...


2

Some quick remarks Follow the naming standards. Property names etc. should not contain anything but alphanumeric characters. No underscores etc. Use meaningful names. "OA" is meaningless, "OA_Plus" is even more confusing. A class name shouldn't be a plural (some exceptions apply): Borders. So many magic strings. "All" ...


6

You can take advantage of the ToObject() method of the JToken. With that your parsing logic could be simplified to this: var jsonContent = File.ReadAllText("/beta/Downloads/test.json"); var parsed = JObject.Parse(jsonContent); //ClientSettings needs special handling var parsedClientSettings = parsed["clientSettings"]; var clientSettings =...


2

If I understand your intent and code properly then you rather have an abstract factory implementation than a factory method pattern implementation. The Abstract Factory pattern, a class delegates the responsibility of object instantiation to another object via composition. The Factory Method pattern uses inheritance and relies on a subclass to handle the ...


3

Here I've two alternative versions of Bryan Boettcher revised code. In his version there are 4 ETL functions and they are running concurrently. In my solutions only the Extract part is running concurrently. The Transformation and Load parts are executed sequentially. Alternative #1 Please bear in mind that I had to use some made up type names (like: ...


6

Database design vs Object Oriented Class Design The code has a look and feel of "database think" not Object Oriented think. Often the database schema naturally overlaps the code classes but that should never be an explicit class design criteria. This code is not object oriented in the least. Reasons why: all classes have no methods - just like ...


3

I think instead of creating an unknown amount of task using PLinq would make it simpler and have control over the number of task inflight/created something like var result = model.Materials .AsParallel() .WithDegreeOfParallelism(8) // whatever you want or leave it out .Select(material => _materials.Get(...


3

Based on feedback from aepot, and lessons from AspNetCore Async Guidance, this is what I refactored the code into: public async Task<Account> RetrieveAsync(AccountId input1, DateTime input2) { Account account = new Account(); await Task.WhenAll( SetAccountMap(account, input1, input2), ChangePayer(account, input1, input2), ...


3

The refactor with an explicit constructor is a drop-in replacement for the original code and would be the direction I would go. The code requiring object initialization requires a change to how objects are constructed and may break code elsewhere in the project. In my opinion, I wouldn't bother changing it unless and until another change is needed. Although ...


12

There is a special use case when the (slightly modified) second approach could be beneficial. Namely when you want to create immutable objects. public sealed class PersonEntity { public string FirstName { get; } public string LastName { get; } public int Age { get; } public PersonEntity(string firstName, string lastName, int age) { ...


3

As for me it looks fine except one thing. You have already completed Task in .ContinueWith block, thus you may use .Result then instead of await and avoid redundant async State Machine. By the way, if you're using async/await, probably there's no continuations needed. Consider this continuations-free implementation of 1st Task.WhenAll() job. public async ...


1

Try to abstract away from helper name, when class is properly named - the whole picture become clear. Low level helper logic shouldn't be visible, instead we just need such usage using (var connection = GetConnection()) { connection.Execute("procedureName", new {A = "A", B = "B" }); connection.CommitChanges(); } ...


5

As pointed out by @iSR5's comment, A DataSet is basically a wrapper around a collection or DataTable objects. It has some other helpful utility methods, but from a performance point of view the only performance issue with your logic is that in the DataSet solution you need to access the table to read from through the indexer that has assumed that there is a ...


3

Don't use public fields. Instead, make them public properties. Use proper naming. What is sp? storedProcedureName will be better. Dictionary is a collection of parameters. Therefore, use the plural name: parameters. Using the AddWithValue method may result in poor query performance. Use the Add method with the exact SqlDbType type specified. The ...


3

Overall this is not a bad technique, there is a code smell though in the number of string concatenations, I'm not going to profile it, but I prefer solutions in string processing that write to string variables as little as possible. What are you missing: There is no support or checking for null values There is no support or checking for a maxLength that is ...


3

I'm not sure if i can review this in details. But I suggest to consider my own implementation of the same. private static IEnumerable<string> Split(string text, int maxLength) { char[] whiteSpaces = new[] { ' ', '\t' }; string[] lines = text.Replace(Environment.NewLine, "\n").Split('\n'); foreach (string line in lines) { ...


3

Thank you for updating your question! In your question you mention the SOLID principle, it isn't just one principle, SOLID is 5 object orieneted design principles. SOLID is a mnemonic acronym for five design principles intended to make software designs more understandable, flexible and maintainable. This will help you design your objects and classes better. ...


7

From MSDN Random class documentation To generate a cryptographically secure random number, such as one that's suitable for creating a random password, use the RNGCryptoServiceProvider class or derive a class from System.Security.Cryptography.RandomNumberGenerator. Random Rand = new Random(); Edit Correction: Using a snapshot of the system clock, the no-...


2

Probably the direction is correct but the class API is inconsistent for now. DateFrom is available, DateTo is not. As for me, the following way looks more consistent. public class MainApplication : Application { public DateTime DateFrom { get; set; } = DateTime.Now; public DateTime DateTo { get; set; } = DateTime.Now; public DateTime ...


2

I think it can be something like this, also it can be improved by using variable names that reflects your business conditions better. private void UpdateAccount(UserModel userData) { Account accountById = _accountController.GetAccountById(userData.Id); Account accountByNumber = _accountController.GetAccountByNumber(userData.Number); ...


1

Properties Seems too many properties public and if they need to be public should use the readonly interface. For example in the tcpserver there is a property called ClientsList that is a List. IT would seem abnormal that an outside class could clear the list or add/remove clients from the list. If the property needs to be public returning back ...


2

well What I would do is created model from the if of validations you have 3 path in the if when accountById is null and accountByNumber is null when accountById is not null and the else Look at the code and see how the TypeCase property let identify each case using your if statement private void UpdateAccount(UserModel userData) { var ...


1

.NET Collections is your friend NOTE: My sample code is intended to show the idea of the thing, not be ready-to-compile code using all those classes you created. My spider-sense tells me they're not all necessary. A custom collection will solve your problems. Inherit or compose with, say List and use the many functions that take expressions (technically ...


2

This code is hard to read, deeply nested and hard to understand. But you already know that. Since you have a lot of binary cases (if-else) I suggest creating a truth table on paper or whiteboard https://en.wikipedia.org/wiki/Truth_table#Binary_operations for your different combinations of input and output. This should help you figure out which cases can be ...


1

There are a couple of cosmetic things that might improve your code. E.g. you can declare multiple pointers in in each fixed statement: fixed (char* ptrDest = m_valueStr) { fixed (char* ptrSrc = value) { wstrcpy(ptrDest + m_Pos, ptrSrc + indx, charCount); } } Becomes: fixed (char* ptrDest = m_valueStr, ptrSrc = value) { wstrcpy(...


Top 50 recent answers are included