52

I've never TDD'd, but don't do that: public class MyDataContext<T> : DbContext where T : class This gives you a context-per-entity, which might work for ultra-simplistic CRUD scenarios, but doesn't scale very well and will quickly give you headaches as soon as you need to deal with more than a single entity type in a single transaction - because that'...


34

Status codes have long gone the way of the Dodo --- made extinct by exceptions, actually. The problem with a status code is that it can be ignored. An exception cannot. Consider these two examples. Status Codes as Return Values First, the "merchant repository" implementing CRUD operations for merchants: public enum MerchantStatusCode { OK = 0, ...


22

If you find a pattern getting too repetitive, you can use @helper instead: @helper ShowIfNotNull(string header) { @if (!string.IsNullOrEmpty(header)) @:<h2>header</h2> } } <h1>@Model.Title</h1> @ShowIfNotNull(Model.Subtitle)


12

I get what you mean by "reversed": As opposed to: Makes sense, at least to me - the way I see UoW/Repository pattern (everybody seems to have their own take at this one, eh?), Entity Framework's DbContext is a unit-of-work, and an IDbSet<TEntity> is a repository. Hence, I tend to agree with having unit-of-work depend on repositories and not the ...


12

The OnException() methods does 2 things. It logs the exception It creates an return value So let us refactor these two points to two separate methods. private void LogException(ExceptionContext exceptionContext) { logger = log4net.LogManager.GetLogger(exceptionContext.Controller.ToString()); logger.Error(exceptionContext.Exception.Message, ...


12

I would say your design is fine. In general I advise injecting IHandleViewModel<T> implementations directly into consumers, because this makes it easier to verify the object graph directly and makes it clearer what the consumer actually depends on. You should do this, unless you regularly inject multiple IHandleViewModel<T>s into the same ...


11

You could just combine the logic into one statement, assuming there is not other things occurring during your if-else blocks and stuff. If there is, then this won't necessarily work for you, and you should post the real code, because a lot of times these problems are subjective to the code. I guess one lesson to possibly learn from this is just to realize ...


11

You're checking that "page does not contain any posts and page number is not one". Reversing the condition is often the easiest way to clarify it. The inverse condition would be "page contains at least one post, or page number is one". I would make it look like this: if (pagePosts.Any() || pageNumber == 1) { return View(pagePosts); } else { return ...


11

There are a number of issues in your code. Naming. Methods should use verb phrases to denote the action they perform and try to be specific. So your DateList method should become something like GetWorkingDates, or just GetDates if kind of dates is obvious in your case. Test naming. Do spend some time on composing a name for your unit test method that ...


10

Let me start out by saying that your test names are actually decent. You have already come to the conclusion that a good test name is one that can convey what is being tested without requiring someone to actually read the test. As with many things, there are three important pieces of information about what a test is doing: What is the thing being tested? ...


10

Not only is this difficult to read, it's also inefficient. Cache your duplicate LINQ lookup to save time and make it more readable. As for using the ternary operator, that's up to you, but I prefer to use a full if statement when the line gets too long. Additionally swap out "" for String.Empty, it shows your intent better, and also makes it more strongly ...


10

Almost. From the MSDN docs on the Disposable Pattern: ✓ DO allow the Dispose(bool) method to be called more than once. The method might choose to do nothing after the first call. This is typically done by introducing a private boolean member variable and a guard clause. private bool disposed; protected virtual void Dispose(bool disposing) ...


9

As Phil Sandler said, the code here is so thin that it's possibly questionable how much value unit tests actually add. However, I would say that their two main benefits are both still relevant: Living documentation of what a unit of code should do Instant feedback for future refactorings While it's much more likely that you'll add to PostsRepository rather ...


8

You are calling queries in a for-loop for each day. Database queries are slow compared to C# code execution. Send only one query to the DB for the whole range of days. You can do the grouping by day either as DB query or query the ungrouped records and group using LINQ-to-Objects later. Also you are querying the lstSalesRepIDs within the loop. Why? The ...


8

We have asked to keep the MVC controller as thin, so we decided to move all the service calling/business logic into the ViewModel class. The concept of the "thin" controller is a guideline to help you avoid having business logic in your presentation layer - it is not as simple as "having more than a little code in controller actions is bad." In moving this ...


8

int num What does num represent exactly? Number of pages? Number of posts per page? Current page number? Properly naming the variable would help a lot in clarity here. var postsPerPage = 3; Turn it into a parameter with default value 3. That way you can always decide later that the user can change this without much trouble. Instead of using 0-based pages ...


8

In my opinion, your view is doing too much work. This is happening because your model is missing a property. If you add an IsSelected property to your model, I think we can simplify the view a bit. public class DeliveryAddress() { public int ID { get; set; } public string Location { get; set; } public bool IsSelected { get; set; } } Now this ...


7

The Context in this situation isn't correct. The Context should have all your dbSets. With the UnitOfWork pattern there is only 1 instance of the Context. It is used by your repositories (DbSets) and by the UnitOfWork. Since there is only a single instance it allows you to call many services, each of which update your context, before calling UnitOfWork....


7

Since it's been several days and this question has not been answered, I will give it a go. I'm saying this not because making Authenticate follow Command-Query Separation Principle is hard, but rather I think this design may indicate a more serious problem with security configuration, and I'm not up to date with ASP.NET. Command Query Separation The ...


7

public interface IUnitOfWork : IDisposable { IRepository<T> Repository<T>() where T : class; void Commit(); void Rollback(); } The goal of a unit of work is to abstract the disposable stuff that's implementing it. By making your interface extend IDisposable, you've made a leaky abstraction - now anyone (including your mocking ...


7

Why the marker interface? public interface IViewModel { // Marker interface } public interface IHandleViewModel<TViewModel> where TViewModel : IViewModel { Task<TViewModel> Handle(); } What you really want to do here, is ensure that TViewModel is a reference type - right? There's a generic type constraint specifically for that: public ...


7

I suggest the following: public class ErrorController : Controller { private const string RedirectLoopCounterName = "RedirectLoopCounter"; private const int MaxRedirectLoopCount = 1; private int RedirectLoopCounter { get { return ((int?)TempData.Peek(RedirectLoopCounterName)) ?? 0; } set { TempData[RedirectLoopCounterName] = ...


7

The responsability of a controller is to create ViewModels and return them. This usually imply some boilerplate code, creating new instances, setting parameters, etc.. Then, you need to return the good view, which also leads to some boilerplate code. Your controllers are thin. Very thin I'd say. I think you're searching too far to refactor perfectly fine ...


7

First point, these extensions are really cool. I don't have much comments to give, your code looks pretty good! The ActionInfo class could be simplified a little : internal sealed class ActionInfo { private ActionInfo(string actionName, string controllerName, RouteValueDictionary routeValues) { ActionName = actionName; ...


7

Index It would appear you primarily do three things here. Sort, Filter, and Page. There's not much to say about your paging, but the sorting and filtering logic clutter the action. Try creating an object that does the sorting and filtering for you. Something like this: Note: I didn't include all the logic you need, however; I did include what I believe to ...


7

Again enums should not have any prefix. Drop all those Enum_! You may like to take a look to .NET Naming Guidelines. Inside the class UserEventLog you do not need to repeat Log prefix for its properties. For example just Message is enough. Think about a Window object: did you ever see properties named WindowTitle, WindowSize, WindowCanMaximize and so on? ...


6

I like the fluent API. Nitpicks first: Drop the #region blocks. They only add clutter, for example #region IHtmlString Members is only confusing, since it's not immediately apparent that GridBuilder implements IHtmlString (perhaps IGridBuilder does? I'm not super familiar with these extensions myself, but I don't think it matters - comments shouldn't be ...


6

I agree with Olivier Jacot-Descombes on the functional aspects of this code, but it also lacks organization, meaningful naming, and consistency, which will make it harder for other readers (or yourself in future years) to follow. Some examples: public static List<DailyTeamGoal> GetListDailyTeamGoals(int teamId) ... List<DailyTeamGoal> ...


6

As far as I can guess you are trying to test whether some method returns a subclass of System.Web.Mvc.ActionResult repeated snippet here: w => w.ReturnType.BaseType != null && (w.ReturnType.BaseType.Name == "ActionResult" || w.ReturnType.Name == "ActionResult") But this snippet fails in three cases: If a class inherits from a ActionResult ...


6

It's a bad idea to modify the session state from inside a view or a partial view. Instead, I would create an alert view model public class AlertViewModel { public AlertType AlertType { get; set; } public string Message { get; set; } } public enum AlertType { None, Success, Error } and use that from inside the view/partial view, for ...


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