Podcast #128: We chat with Kent C Dodds about why he loves React and discuss what life was like in the dark days before Git. Listen now.
12

throw ex; This is a fatal mistake. It'll create a new stack-trace and you won't be able to tell where the actual exception occured. If you want to rethrow it then use just throw; without ex.


10

There is nothing inherently wrong with having multiple using statements. It keeps the lifetime of objects to the minimum which is not a bad thing to do. Another point is that I'd possibly re-factor the error raising: You always prepare the error message with the same parameters except for the actual message. This could easily be encapsulated in a little ...


8

As a habit, I prefer to use using statements for only the granular pieces where they are applicable rather than wrapping the entire code file. The reason for this is that though you are calling dispose when the using block ends, a managed object (for example, database connections) will generally be placed in a pool for later use. In this way, you don't ...


6

Please use Brackets It isn't a necessity to make the code compile, but it is standard usage in C# so this if (authenticationData == null) using (var responseManager = new ResponseManager()) return responseManager.PrepareMessage( HttpStatusCode.Unauthorized, "No authentication ...


6

Simplified, examplified, trimmed code is frowned upon on this site; I'm surprised this question hasn't received any close votes yet. I don't see anything blatantly done wrong here - I like . On the other hand if the client code you've shown is cohesively written in a specialized service (class) with a clear, focused interface that your ViewModel receives ...


6

Let's start with the BUG. if (entity.Count() == 0 || entity == null) If entity is null, then you'll get a NullReferenceException when Count is called, so there's currently no possible way the second half of this statement will ever be called. I think you meant to do this. if (entity == null || entity.Count == 0) Which works, but isn't great. I would ...


6

In Get() you do not need to wrap the call to GetAllCustomers() into a Task because it's already asynchronous: public async Task<HttpResponseMessage> Get() => await GetAllCustomers(); There is, however, something else to do: GetAllCustomers() should be renamed GetAllCustomersAsync() to inform that it's an asynchronous method. ...


5

Naming conventions. In C#, types are named using PascalCase. Rename: OM_Role to OmRole DAL_Role to DalRole rolesAPIController to RolesApiController and so on The same goes for properties - so tblRoles should be named TblRoles Coding Style You could mark your private field _role readonly. People often also explicitly mark them as private though it's not ...


5

Style Be consistent in your style. If you use braces {} for single if statements then you should use them always. Using guard conditions help you to save horizontal space. NameValueCollection mapQuery = UriExtensions.ParseQueryString(requestUri); if (mapQuery == null) { return target; } string fields = mapQuery.Get("fields"); if (String....


5

You can wrap the single entity into an IQueryable. Something like this should work: SingleResult.Create(new[]{entity}.AsQueryable()); You could write a helper function that uses this trick to convert from a single T to an IQueryable<T>. There may even be a built-in way of doing this.


5

Thing that makes me more perplex is error-handling. First of all you're catching all exceptions, it may hide subtle bugs unrelated to web request itself (for example wrong parameters you supply in your own code, StackoverflowException, OutOfMemoryException and so on). You're swallowing exceptions, you just log them and return null (which is little bit ...


5

Typically in an ASP.NET app (and in fact in any app), you should only catch and handle the base System.Exception in a single place as close to the application entry point (or in this case request entry point) as possible. This means you don't have to duplicate exception handling code in every action method, and helps ensure you always return a consistent ...


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) { ...


4

I see this pattern from time to time but I don't like it: GetJobBySurnameVRN or GetJobById. That's what overloads and perhaps named arguments are for: all you care about is the GetJob part, everything else is just one specific way of retrieving the data but does not have to actually be a separate method rather than an overload. I would change it to GetJob ...


4

I'm just going to comment on one line: if (dbUser == null || dbUser.Password != user.Password) As you confirmed, you're currently storing passwords in the clear. Encrypting passwords is not the right solution either. Cryptographic Right Answers (2009, Colin Percival, author of scrypt) Password handling: As soon as you receive a password, hash it ...


4

Now I will answer myself, hopefully someone else has use of this. When it comes to the server side, I'll let it be like it is. Using HttpGet could be useful for caching so that's a good thing. The client code works but it's a bit messy so decided to clean it up with some simple helper classes. In these classes I'm using HttpClient instead of WebClient. ...


4

I cannot think of a clean way to extract the validation logic. All validation logic should be in an appropriate "validation service" class as @meWantToLearn said. I assume something like this is possible in FluentValidation: public class PollVoteValidator : AbstractValidator<Poll,VoteInput> If not then make a simple class that contains both of ...


4

I know this is an old post but I tried doing something similar recently so thought I should share it here. The code looks great but it still returns unwanted properties with 'Null' values. What I have tried to do is serialize only those fields that are needed. public class ShouldSerializeContractResolver : DefaultContractResolver { protected override ...


4

What does the using do? It is a terse way to call dispose on the object at the end of a given scope. So first, make sure you actually need to call dispose on the object at all, because it's completely possible the GC will take care of the object automatically. If you're concerned about garbage collection due to the async (server-side) nature of the request, ...


4

Do not use a public field; instead, make it a property: public bool isInitialized = false; Your code is barely above 100 lines, you don't need regions and you shouldn't use them anyway. Local variables like var Patron should be camelCase. Don't use Boolean, etc., instead use the alias (bool,...). This: Hold hold = new Hold(); hold....


4

Short answer is maybe. If this is the only code that updates it then you will be fine but you are making a gateway that only one person can update at a time even if they have different codes. If your site gets big you will have performance problems. You should have your database handle the concurrency when possible. If using SQL you should use the ...


4

It's a very bad idea to call the controller a FileSystem and inform the user that he's going to be working with well, a file system. He should not know how you are storing the data he uploads. You should not reveal this implementation. A Web-API is a resource and should be seen by the client like one (whereever possible), this means your controller should ...


4

I think one of the most common errors in today times, is for example handling potential errors, as you try to do here: [Route("api/FileSystem/Upload")] [HttpPost] public string Upload() { try { // ... code } catch (Exception e) { Console.WriteLine(e); return "File upload failed"; } } Why is this bad: You ...


4

The naming convention Is… implies that the function is a predicate, returning either true or false. If you wanted to throw an exception when the database is down, then you might name the function RequireDatabaseUp(), returning void, instead. But that seems silly: why not just try performing a real SQL operation instead? In summary, if you want to provide ...


4

The intention When I have all matching objects - I pass them to a "Paging" method within the service (I think I have to do this in the service rather than the repository because a generic repo paging method won't work across some resources e.g "orderBy" a property nested within a class in an aggregate object). I'm apprehensive of your design decision ...


4

The first thing to do is to remove duplicated code. If you do not need further processing DeserializeObject<T>() returns T and you do not need any mapping, you simply can return its result. That code is also shared between A and B then you can simply introduce a separate function: private static bool TryDeserializeObject<T>(string json, out T ...


4

in your case I would try to avoid subscribing to events as this couples the projects and references and prevents the garbage collector to clean up the unused service. if you are creating new services per use (probably you do) you would have a list of services which stay in memory and GC cant collect them because of the event subscription. Instead you can ...


3

Avoid duplicated magic strings, like "http://www.ortund.com/". Assign to a constant instead. This snippet appears repeated multiple times: client.BaseAddress = new Uri("http://www.ortund.com/"); client.DefaultRequestHeaders.Accept.Clear(); client.DefaultRequestHeaders.Accept.Add(new MediaTypeWithQualityHeaderValue("application/json")); It would be better ...


3

You don't have a people, you have a person. Saying if (person.IsMain == true) is exactly equivalent to if (person.IsMain) Updated code below. public HttpResponseMessage SetAsMain(int id) { People person = repository.GetById(id); if (people == null) { return ErrorMsg(HttpStatusCode.NotFound, string.Format("No people with ID = {1}", id)...


3

Your implementation is technically correct, but... There is no benefit in using Task.Run in an web application. All you're doing is deferring work from one thread to another. So, in fact, not only is there no benefit, you're actually adding overhead. I'll quote Stephen Cleary, because I could nevet put it better: There are (at least) four efficiency ...


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