We’re rewarding the question askers & reputations are being recalculated! Read more.
9

I fear that the credentials could be sniff by some attacker if they are listening to during the connection process. If https is not enabled, the password (if there is no protection) could be seen by any attacker during the HTTP requests. This point is not directly related as to how you implemented your code, but it will go a long way as to how your code is ...


8

Design if (response.IsSuccessStatusCode) { string content = await response.Content.ReadAsStringAsync(); result = JsonConvert.DeserializeObject<IEnumerable<WorkItemDto>>(content); } if (!response.IsSuccessStatusCode) { throw new Exception((int)response....


7

You can create a custom authorization class inheriting ActionFilterAttribute public class AuthorizationFilter : ActionFilterAttribute { public override void OnActionExecuting(ActionExecutingContex filterContext) { //Write you authentication logic here . you can use Request headers,cookies ,..etc } } Then decorate your action method with [...


7

It's looking really good. Some things I might consider that aren't necessary but could help move the code to some better abstractions and hence allow for easier unit testing of features include. Abstracting your service away from UI related actions by removing the dependency on HttpContextBase. It looks like all you are using it for is to obtain the ...


6

Extract that logic into a data layer. Minimally, extract a private Upsert method. Create an extension method for cloning the properties from one of your models to another. Stop this nonsense. success.Append("<p>"); success.Append(Tools.Format.Title(model.Title, model.Subtitle)); success.Append(" has been created and can be accessed from the <a ...


6

@Nick already has covered some important stuff, so I'd add my two cents. Nick pointed out that : if (response.IsSuccessStatusCode) { string content = await response.Content.ReadAsStringAsync(); result = JsonConvert.DeserializeObject<IEnumerable<WorkItemDto>>(content); } if (!response.IsSuccessStatusCode) { throw new Exception((int)...


5

Nested If-Statements: Nesting If-statements tends to lead to code that's harder to follow at times. So when you're able to reduce the amount of nesting that you do by returning early (once again this isn't always possible but it can be helpful to reduce nesting and therefore improve readability). Since you do nothing after the ModelState.IsValid if-...


5

Semantically, HTML button tags are supposed to represent a user's action, whereas the link tags are supposed to take care of the navigation between pages. That said, as @user1320170 pointed out in comments, your tag should be a link (< a > tag.) You'll notice it's much clearer this way. Apart from that, your code is good, using @Url.Action("Index", "...


5

Your brace style is inconsistent. Allman style is usually applied in C#. public class AppUser{ // snipped } public class AddressDetails { // snipped } Longtitude is a spelling error, it should be Longitude. Underscores are usually only used for fields; properties should be PascalCase: protected AppDB _db { get; set; } protected AppUser ...


5

public async Task<ActionResult> UpdateHandicap() We add the Async suffix to methods that are marked with the async keyword. This is a naming convention that as you'll see in a moment Entity Framework follows too. var results = db.Users.ToList(); You can use the Users directly in the loop, you don't have to call ToList first. foreach (Users user in ...


5

Too many Language Repetitions You use the "Language" key a lot. What if you decide to rename it sometime? You'll need to search everywhere for this string. It'd be better if you encapsulated it or used a constant. I prefer extensions so it might look like this where you try to get the cookie and if it is not null ? then you get the value. public static ...


4

public virtual async Task<ActionResult> _projectedit(ProjectCreate model) Based on the Task-based Asynchronous Pattern (TAP) one should add the postfix Async to the methodname for async methods. Based on the NET naming guidelines methods should be named using PascalCase casing. Having an underscore prefixed methodname is really unusual. Underscore ...


4

I'm no big fan of using partial classes because I find it less readable. Like the other commenter I think you should look at the repository pattern for a cleaner solution. It seems a bit odd that you give Student the responsibility to save itself to the context. Something in the same neighbourhood is the UnitOfWorkPattern. That said the DbContext is an ...


4

I can see repetitive code which you can fix easily.. Before //Remove old tokens: var oldRefreshToken = user.Claims.Single(c => c.Type == AuthenticationValues.RefreshTokenKey); var oldAccessToken = user.Claims.Single(c => c.Type == AuthenticationValues.AccessTokenKey); var oldExpiresAt = user.Claims.Single(c => c.Type == AuthenticationValues....


4

Let's take about the ManageViewModel for a moment, because it has a lot of things you can do better. public bool IsAdmin { get; set; } public string Role { get; set; } public string GetRole(bool isAdmin) { if (isAdmin) { return "Admin"; } return "Employee"; } With C# properties you can really simplify how this whole thing goes: ...


4

I can be very short about this: Single responsibility: a controller shouldn't also have Unit of Work responsibilities. The context itself is the perfect Unit of Work. You don't always need SaveChanges Use dependency injection. I like to see controllers as light-weight doorways to web-independent service methods. The services have their own DbContext and ...


3

Here is a pattern I've used successfully. However, you need to be aware that the X-FORWARDED-FOR header may contain a comma separated list of ip addresses. This solution also helps with protocol sniffing, because most secure sites will need to force the web browser to SSL, and if you are using SSL OFFLOADING, you'll need to use the x-forwarded-proto header ...


3

You need to work on your naming. You have lots of incorrect casing e.g. local variables in PascalCase but the content of the names isn't as good as it could be either: var logedInUser = User.Identity.GetUserId(); loged is a typo; it should be logged. It's also not a user - it's an id! The name would be better as loggedInUserId. You should read the official ...


3

I think methods for interacting with db classes, manipulating and filtering data belong in the model. So if there is already an appropriate model class for the controller then move the code there or create a new model to service the controller. I would create a model TicketsService and a method inside it to return tickets; something like the following - ...


3

It's odd that the TaUsers result column is NVARCHAR(100) rather than a nullable number. The setting of the local variables (@ClassroomLearning, etc.) could be a bit more succinct. I don't see any reason to use a temporary table; it just complicates things. The whole procedure is just a complex query, and should be written as such. If you help managing ...


3

I think "BadRequest" is OK for that case. Alternative options are "PageNotFound" or "redirect to index". Some other points about your code: 0 is usually a valid ID so I would use -1 as default, even if that is not true in your case. You could check for null or empty first to simplify the code . public ActionResult Edit(int id = -1) { if (id < 0) ...


3

Asserting if (viewModel != null) Assert.IsInstanceOfType(viewModel.GetType(), typeof(DraftViewModel)); You should use another assert and not an if Assert.IsNotNull(viewModel); Resharper even suggests this option: Test name Create_DraftFacade_ViewResultWithDraftViewModel You might also want to reconsider the naming of your tests. Usually we use this ...


3

Is this secure? No. It isn't secure because you can change anyone's email address on the last step. To do so, all I need to do is change the emailId stored in the hidden field (for example, using my browser tools). In the final step you must verify the token. You don't need step 2 at all. There are two requests you need once you have your reset token. ...


3

Global static state makes your code untestable... I would define your car make lookup as: public class CarMakeLookup { public CarMakeLookup(IEnumerable<(short Id, string Make)> data) { Makes = data.ToDictionary(d => d.Id, d => d.Make); Ids = data.ToDictionary(d => d.Make, d => d.Id); } public ...


2

One addition to what has been said: It's not enough to make sure that the login is done using https, as you don't want the authentication cookie to be submitted during any non-secure requests, as it could be intercepted by anyone listening to network traffic. So, you also need both of the following conditions to be true: The authentication cookie needs to ...


2

Well, my first attempt is to suggest injecting a mapper into the controllers you want to do this for (assuming you are using a DI framework. If not anyone of the popular choices out there will meet your needs. AutoMapper, Unity, Autofaq etc). public interface IMap<TTarget, TSource> where TTarget: class where TSource: class { TTarget Map(...


2

To add my 2ct worth: What you have started to implement is the Active Record pattern. We have used it in a Winforms/WPF application and ended up changing it to a Repository pattern due to the fact that it was annoying to unit test and we started having session scoping issues (but those were probably a result of bad application of the AR pattern rather than ...


2

I'll address one specific aspect - performance. Considering methods such as getChangeDates - they should return awaitable tasks (especially the ones involving asynchronous operations such as fetching from the database). The flow of getItemHistory is completely synchronous and runs in O(n) time complexity. If each one of the nine getWhatever methods takes ~...


2

At the end of the day, only you can tell what performs best. But there are two things that in most cases will help improve performance. 1. Reduce the number of queries. Essentially, you do DAL.group_table group = DB.group_table.Find(gid); sharedFolders = group.sharedFolders_table.ToList(); And in the view, for each sharedFolders_table you diplay ...


2

Yes you are on right track. Controllers are not much more than traffic directors, they signal the repository/business layer to get or put the data. Then they return the content to the View. They also disallow any posting or getting of data when the state is not valid. Note: You can implement both client side and server side validation. If you use Entity ...


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