12

Disclaimer: if this answer feels like it gets less patient and more "to-the-point" as it goes on, I'm really tired as I am / was writing this and I can't English / logic right now. I promise I mean the very best with this, and I'm having trouble even typing at the moment. This is actually fairly solid code for a C# beginner. Oh boy, there's a lot to talk ...


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.


9

This sounds exactly like a desire for Dependency Injection (using an IoC container). ASP.NET MVC is perfectly setup for this already with plenty of containers out there for you to use! You can still have a base constructor take arguments if you wish but I would try and limit how many. So your controllers would look like: public StudentController(...


9

Creating a model just to check the parameters is overkill and not idiomatic in Rails. If you need some extra logic, use the existing model. In this case, I'd make Post#tagged_with accept a nil value which wouldn't filter anything: def Post scope :tagged_with, ->(tag) { tag ? <code to filter> : all } end class PostsController < ...


8

This is a funny question because One of the most readable Angular submissions on CR There is probably too much logic in the controller indeed Some observations: It seems the controller knows too much, it should not know that 5 is the maximum amount of items, or that 1 is the minimum amount of items to be kept It seems silly to pass the list to list every ...


7

This switch should be an if statement instead. if (input == null) // we didn't get an input from GetCustomEditorInput { switch (property.PropertyType.Name) { case "Boolean": input = new ExpressFormsCheckBox() { FormName = FormName, ...


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


6

I would make some changes to getFirstUpper(): public String getFirstUpper(String str) { Integer x = str.length(); str = str.substring(0,1).toUpperCase().concat(str.substring(1, x)); return str; } The name x is not a very good name as it's only a single character. I'm not even sure if a separate variable is needed for this, but if so, consider ...


6

Jonah's answer really doesn't address the architectural issue with your code. Yes, he makes valid points about your presumptive casts, but honestly, if this is what your company is talking about when they say your code has "architecture issues", I'd be a little amazed. The primary problem that sticks out to me is this: etc.managedObjectContext = self....


6

I would suggest using a service object. I would like to see what others think about this. It looks like there's too much going on for the controller. The controller should be responsible for execution and value return. The model for processing the object. The service for wiring up the object and giving it prepared params to do its job. This is not tested, ...


6

Bearing in mind that you claim that you do not to maintain this code anymore1, there are a few suggestions I would have about this code: define functions and accept parameters- that should allow the nested functions to be moved out... You may be able to utilize Partially applied functions. catch rejected promises The code calls $q.all() with 6 promises, yet ...


6

Here I will discuss only about your API, I think something may also be changed in your domain model but I don't see enough code to suggest anything really meaningful. You have some code duplication you can avoid, extract this: private async (string UserId, IEnumerable<string> Roles) GetCurrentUserAndRole() { if (!User.Identity.IsAuthenticated) ...


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


6

Errors An assumption: you never ever want to return a 500 HTTP status code (Internal server error). It may leak information you do not want to expose and it's not informative enough. Do not throw any exception: return the correct HTTP status code and eventually supplement it with the an appropriate error message. For example if SaveAll() failed because a ...


5

Ok, so I just recently arrived at a way of doing this sort of thing that I actually like so let me share. In my opinion, the if statements aren't really the problem, its more that they are not really pertinent to controller code. Actions should strive to contain code only about major decisions of what code path will be executed. Solution: You take ...


5

Just put if, not else if so you can have a cumulative multi criteria search, and one condition in every if. So every condition is easy to read, and if more than one criteria is filled, they will cumulate fine. if (!String.IsNullOrWhiteSpace(searchString)) books = books.Where(b => b.Name.ToUpper().Contains(searchString.ToUpper()); if (!String....


5

Some remarks and refinements of previous answer but too long to put in the comments. Instead of <button ng-click="addToList()">Add to List</button> I would make it slightly more expressive and re-usable: <button ng-click="addToList(todo)">Add to List</button> That is also consistent with remove($index), which I would rename to ...


5

Given the code provided the thing that stands out to me as a potentially poor design is the way you are obtaining references to controllers. Let's walk through why that might be a problem and what at least one alternative would be. (UITabBarController *) self.window.rootViewController; Here you obtain the window's root view controller. All you really know ...


5

The actions I'm wondering about are members, add_student, and remove_student. They work just fine, but they aren't RESTful [...] True, they're not part the usual CRUD actions that can be expressed with HTTP verbs. But then neither are the default new or edit actions, so it's not like going beyond CRUD is automatically illegal. Even so, associations do ...


5

if (User.Identity.IsAuthenticated) { var username = HttpContext.User.FindFirstValue(ClaimTypes.NameIdentifier); var user = await _userManager.FindByNameAsync(username); var roles = await _userManager.GetRolesAsync(user); artistCardDtoCollection = _artistsService.GetAllArtists(user.Id, roles.Contains("Admin")); } This and the accepted answer ...


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


4

What is a router used for? Translate one address to another one connection two different network to eahc other depending on rules. It will never instantita a browser to display for example a HTTP request. Your class doing to much and much lesser than it should Your router should only deal with the input address and not instantiating a controller and other ...


4

All in all, I'd say your controller is looking pretty good. There are no real issues concerning separation of concern, just like there were nonein your previous code I reviewed. I have noticed one or two things that I'd describe as being odd, and even slightly worrying: Possible violation of the Liskov principle, or basic inheritance rules in general (...


4

@GaryStorey hit on the simplest solution: Just pass your variables to the function: // ... likePost: function () { var $likePostEl = $(this), $postLikeCountEl = $likePostEl.children('#likeCount'), postLikeCount = $postLikeCountEl.data('count'); var request = $.ajax({ url: '/posts/like', data: { post_id: $...


4

Naming In update_count, the variable v is very poorly named. It seems that meals_today would reflect its purpose, and that would be a much better name. Simplifications In update_count, you check if v is None a few times. You could simplify the code by initializing its value to 0 instead of None, so: meals_today = 0 Then, instead of this: ...


4

What I noticed is that there are very few differences between what's happening within the sections that the 'dancers', 'staff', and 'volunteer' messages take you through. Because the only thing that really changed between the three was the word within the message itself, I decided to combine them and use the message variable. def process_message(message, ...


4

So, I think that this would be a good opportunity for a generalized service pattern. In general fat controllers are a fairly bad idea, in fact fat methods anywhere are generally a fairly bad idea. Unfortunately it looks like the Wit ruby client prefers closures to more object oriented patterns. Here are some things that I'd do: Make a strong distinction ...


4

user.assign_attributes(params) user.save model.assign_attributes(hash) allows you to assign a key value hash to your ActiveRecord object. The only caveat is that the keys passed must correspond to your columns or an exception will be raised. You will need to call .save to persist the changes. If you want to assign and save in one fell stroke, use model....


4

Now, to my best knowledge, $this->request in PHP always points to the HTTP-request when a HTTP-request has just been made My guess is that you are thinking of the super-global $_REQUEST? That is not the same as $this->request (unless somehow that instance variable gets set to something else)... Is it okay to just ignore the warning or is there a best ...


4

I think there is more to discuss here then business logic inside your controller. I'm going to generally keep it to a high level of discussion, but feel free to comment and ask for specifics. I'm going to break it down like this: Security issues Architecture issues Business logic issues Formatting issues Here we go! Security These two lines right here ...


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