7

I'm a quite a junior programmer, but happy to learn! :) I put together a WebApi based application and I sent it to be reviewed by my colleague. I got it back with the opinion the way I use partial classes hide controller complexity and size. I'm really happy to hear his opinion (he has been a developer for more that 15 years) because I respect him. But I like to cross-check opinions, viewpoints to see as many aspect of a case as possible.

You can find the examples and screenshots below about the code.

The reasons why I organize my stuff are the next:

  • easy to find the controller method in solution explorer
  • one file ( one partial class ) contains only a single method and its internal utility methods (if there is any). If a utility method is used by multiple controller methods, then I organize them in a common place
  • a controller is responsible for a single endpoint (in the example you can see the Environment controller which is responsible for environment sub-domain)
  • a method is never longer than a common screen size for two reasons: a, I hate scroll up and down, b, controller business logic is extracted to a separated library called Application, the controller responsible only for call the application method and return the result.

My colleague mentioned that, partial classes are awesome when a class implements multiple interfaces and you can separate the interface implementations into partial classes.

My questions:

  • it is enough information and code sample I provided?
  • it is a good approach I have?
  • if not then where are the weak points of it and why?
  • what is the common pattern in these area, however, code review is rather a common set of opinions and culture of a team/company
  • where is the healthy trade-off between readability/manageability (according to the code review result I pushed this part a little bit to far) and hiding complexity, violating responsibility rules

EnvironmentController.cs file: it is responsible (it contains only) for dependency injection related setup (constructor injection), listing fields and having the RoutePath value.

[RoutePrefix("Environment/Environments")]
    public partial class EnvironmentController : ApiController
    {
        private IEnvironmentApplication environmentApplication;

        public EnvironmentController(IEnvironmentApplication environmentApplication)
        {
            this.environmentApplication = environmentApplication;
        }
    }

EnvironmentController.AddNewEnvironment.cs: responsible only for AddNewEnvironment method and possible internal utility methods, if there is any

public partial class EnvironmentController
    {
        [HttpPost]
        [Route("AddNewEnvironment")]
        public HttpResponseMessage AddNewEnvironment([FromBody] string environmentViewModelString)
        {
            try
            {
                EnvironmentViewModel model = JsonConvert.DeserializeObject<EnvironmentViewModel>(
                    environmentViewModelString,
                    new JsonSerializerSettings { NullValueHandling = NullValueHandling.Ignore });

                EnvironmentContract result = this.environmentApplication.AddNewEnvironment(model);
                return this.Request.CreateResponse(HttpStatusCode.Accepted, result);
            }
            catch (Exception e)
            {
                return this.Request.CreateErrorResponse(HttpStatusCode.BadRequest, e);
            }
        }
    }

How it looks like in solution explorer.

enter image description here

migrated from codereview.stackexchange.com Jun 2 '17 at 14:44

This question came from our site for peer programmer code reviews.

  • 6
    I feel like this is not a code review question, but a general software design question that is better suited for SE.SoftwareEngineering. But for what it's worth, I definitely would not want to have to open 6 files side-by-side just to see the complete implementation of otherwise trivial class – Nikita B Jun 2 '17 at 13:08
  • 3
    Agreed, not a Code Review question, but good and well written question anyway. It would infuriate me to have a class partitioned as you have done, because not being able to see the class's functionality all in one "go" hinders rather than aids comprehension. Your classes should be small enough anyway that they don't need to be split out into multiple files (except in some cases e.g. designer-generated code). Also, you want to see the methods more easily? Expand the class file in Solution Explorer! – 404 Jun 2 '17 at 13:10
  • If you're keeping things in the same file, a very useful tool I use is the #region/#endregion keywords (access it with CTRL + K + S ) which can "hide" details - which works wonders to make classes more VISUALLY accessible and navigable. please do not misuse the keywords to avoid refactoring, they should be a purely visual tool. – BKSpurgeon Jun 2 '17 at 13:27
  • If you click the down arrow next to the CS file you'll get it organized how you're organizing it now. Also you can use the drop boxes at the top of the file to find the individual functions. Use your tools, don't do it manually. – Kevin Fee Jun 2 '17 at 15:27
  • @BKSpurgeon I use #region rarely. I heard people has controversial opinion about them. – SayusiAndo Jun 2 '17 at 15:31
4

Your approach is not common practice and may raise a few issues, since Visual Studio is optimized to work a different way.

easy to find the controller method in solution explorer one file ( one partial class ) contains only a single method and its internal utility methods (if there is any).

Visual Studio provided dropdowns in the top of the code pane that allow you to quickly find a method. Also, VS supports regions that can be named, nested, hidden, and shown as needed.

If a utility method is used by multiple controller methods, then I organize them in a common place

What about a utility method that was previously defined in one of your method files, but later you discover you need to call it from a new method? You'd have to move it. Not only is that a PITA, but it will break history in your code repository.

a method is never longer than a common screen size for two reasons

No reasons necessary; short methods are a good practice in general. One way to keep methods short is to break them into smaller methods. But-- using your approach, there is actually a disincentive to break up longer methods, because the developer will have to add a new file each time. God help a developer who is restructuring code and wants to to be able to add/remove/restructure a whole lot of methods at once.

The only advantage I can think of is this: in the "one file" approach, if a developer decides to move a method around (e.g. move it from the bottom to the top) it can cause code merges to get confused. If there is only one method per file, this is a non-issue. But-- on the other hand, if a method name changes, under your system the file name would change as well, and merges might get skipped entirely.

4

I'm sensing we'll start a religious war over this, but I'll state my opinions over the matter.

What you're doing is basically reducing the complexity of a single class (in this case the EnvironmentController) by increasing the complexity of managing the project, by spawning several files in a single folder.

If you have few controllers, I don't see any downside. If you have several controllers on that application, this can become messy quickly.

I've got a few projects with over 25 controllers, some of them with over 10 methods. Using your approach, that's over 250 files to manage, where today I manage just 25, and modifying the methods in the IDE (VS.2015) isn't hard at all, since most of the methods are small like yours (less than one page).

One extra thing that makes me a little bit uncomfortable with the partial class approach is the fact that I like to know everything the code does in a single look. I like to code for reading, and having everything in a single file makes my life easier.

In the end, that may be a personal preference. If you and your team are comfortable with the partial class approach, then by all means go for it.

Just try to stick with a single approach over your codebase. It'll be hard for a newcomer to get used to several different patterns of code structure just because one developer has a different caprice then another. Keeping a single style helps everyone on the team to communicate.

Remember that I said that I have a few projects with over 25 controllers ? That was done by a team of 6 developers, 2 being seniors, 2 juniors and 2 in the middle. At the beginning we all agreed on a format to organize the code, split the controllers, views, models, DALs, etc... and now everyone can easily correct and extend functionalities of each other code without problem, even with different coding styles.

Side note: After almost 5 years working together, it's pretty clear to us how each other code by just looking over the style of each one. Each developer kept his own style and personality when coding, but maintenance isn't a burden for anyone on the team, because things (methods, actions, business rules, etc...) are easy to find.

2

This might not be a BIG problem, but what your reviewer told you is correct: you could be hiding the complexity of the controller by doing this (and maybe some bad practices). And what was already mentioned here:

  • This is impractical for big controllers (1 class with 10 methods, vs 10 partial classes)
  • It is not a common practice.

Most of the time you'll want your code to be easily understood by others. And if you have a controller that has too many methods, maybe the best idea would be to review and check if you are putting too many responsibilities in one place.

A controller is not a "normal" class, as it (ideally) won't contain any actual business logic of your application (most likely will call other interfaces) and that means that your methods are not going to be big. With your approach you'll end up with a bunch of 5 or 6 line classes.

A better way to do this could be to divide by types of responsibilities, putting anything that inserts/adds/updates in one place, and all your Gets/selects in another. That is not a bad approach, but if you are doing things right in other areas, and your methods are concise and small, you won't need to separate.

1

This is similar to a question about using #region's in your code to hide stuff. The real problem your colleague is hinting at could be related to the controller doing too much. If one action method needs a bunch of utility methods to do it's job, and those methods are not related to handling the HTTP request, then you are missing classes - be they "service" classes or "domain" classes or whatever you want to call them.

Basically your coworker is saying that you could be violating one or more of the SOLID principles of object-oriented programming.

Stuffing too much in one​ class, and then fracturing that logic into multiple files will make it a night mare to refactor.

Partial classes were a way to separate auto-generated code from human written code. It was never meant to "fix" a class that has gotten too big and does too much.

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.