Hot answers tagged

13

I suppose the simplest thing is to go through the principles: SRP - Cannot see any 'too big' things in your example OCP - The example is small enough and you are not using class inheritance that I cannot think of how this would apply LSP - You only have one implementation of each interface so hard to say ISP - Your interfaces seem to serve a single purpose ...


7

Welcome to CodeReview! I hope you get some good feedback. Absolute paths string ID = Request.QueryString["id"]; string imageFile = "\\\\server\\c$\\inetpub\\wwwroot\\web-app\\images\\" + ID+ ".jpg"; This has a few issues. First, it's typical for a server to be in a "jail" where it shouldn't have access to the ...


6

IMO returning IEnumerable<T> from an API is a violation of "L", as running GetEnumerator() a second time (or foreach, or Count()) behaves differently for different kinds of IEnumerable<T>s. It may fail, run forever, or have expensive side-effects (like running a database query). Instead commit to returning an in-memory collection, and ...


6

Quick remarks: Why do you convert a string to a string: Convert.ToString(ConfigurationManager.ConnectionStrings["MyDBConn"].ConnectionString);? It's even called a ConnectionString. Why do you use Dapper and then don't use it properly? Why return a DataSet? Why use SqlMapper and DynamicParameters? Why make life so hard for yourself when Dapper is ...


4

This implementation is quite far from a production-ready CRUD API. Just to name a few missing things: Presentation and Persistence layer separation Controller should deal with HTTP related stuff, not with databases Proper status code handling (see the next section for more details) Model validation Error handling for example: what if database is not ...


4

You could easily turn your method into a one-liner expression-bodied method with a few tricks: use the _ discard variable instead of the explicit parsedDate variable (also, it doesn't need initializing to new DateTime()). use the conditional operator since you're returning one of two ValidationResults. value.ToString() can be passed directly into ...


3

Consider not using System.Drawing to manipulate images in ASP.NET: Classes within the System.Drawing namespace are not supported for use within a Windows or ASP.NET service. Attempting to use these classes from within one of these application types may produce unexpected problems, such as diminished service performance and run-time exceptions.1 An ...


3

No need to manually create the DbContext. Since you have already registered it with the service collection, resolve it as needed. AddHttpClient has an overload that provides access to the service provider. services.AddHttpClient("VendorAPICall", (sp, c) => { c.BaseAddress = new Uri("https://api.vendorname.com/"); c.Timeout = new TimeSpan(4, 0, ...


3

If you add IWebHostEnvironment as a dependency to the middleware class, you can simplify the environment check by this: if (webHostEnvironment.IsDevelopment()) { }


2

Quick remarks: _https_port: do not use underscores in variable names etc., except at the start. _https_port = 80; The HTTPS port is most definitely not port 80. The logic to fill redirectUrl and then redirect is duplicated. It should be a method that gets called. I'd move the "compile new host value in these specific cases" logic to a method and ...


2

This code is pointless since it creates async state machine while the code you execute is actually synchronous code. So eventually you get a performance penalty for the creation of a state machine but no reward of asynchronous execution. Consider using async and await when you're dealing with IO operations. Usually, such methods already provide asynchronous ...


2

As requested, I moved my comments into an answer. Do not add the IMapper directly to the DataReader because you then give the DataReader 2 purposes (breaking S):Reading data and mapping it to the DTO. Your DataReader is used to read the data. DTOs are created just before sending it over the wire. If your GetUsers() is within a controller you should inject ...


2

Let me reflect to your other question, which is related to the good old ServicePointManager. In the early versions of .NET Core they tried to get rid of the ServicePointManager and the related HttpWebRequest class as well. Instead they have introduced two new classes: WinHttpHandler and CurlHandler. Former should be used in Windows environment and latter ...


2

I guess you'll find a lot of redundancy in the code since its been held by the controllers. Your idea of making a service is a good idea, you may need to add different services, but at the end you'll have services that can be reused, avoiding code redundancy. This would help ease things more and more. So, continue in this approach. Interface or abstract, ...


1

From the separation of concerns perspective it seems fine that you do not want to validate against emptiness. On the other hand, one can argue that why do you treat an empty string as a valid name. My suggestion is to create a ctor with a parameter, where the consumer of the attribute can define how should it behave in case of empty input. public class ...


1

I think other answer had some great points, also I would use fluent validation instead so your model does not need to have dependencies on validation attributes. https://docs.fluentvalidation.net/


Only top voted, non community-wiki answers of a minimum length are eligible