class modifier issues in C# with "private" classes

Question

I had a class that had lots of methods:

public class MyClass {
    public bool checkConditions() {
        return checkCondition1() &&
               checkCondition2() &&
               checkCondition3();
    }

...conditions methods

    public void DoProcess() {
        FirstPartOfProcess();
        SecondPartOfProcess();
        ThirdPartOfProcess();
    }

...process methods
}

I identified two "vital" work areas, and decided to extract those methods to classes of its own:

public class MyClass {
    private readonly MyClassConditions _conditions = new ...;
    private readonly MyClassProcessExecution = new ...;

    public bool checkConditions() {
        return _conditions.checkConditions();
    }

    public void DoProcess() {
        _process.DoProcess();
    }
}

In Java, I'd define MyClassConditions and MyClassProcessExecution as package protected, but I can't do that in C#.

---

How would you go about doing this in C#?

Setting both classes as inner classes of MyClass?

I have 2 options: I either define them inside MyClass, having everything in the same file, which looks confusing and ugly, or I can define MyClass as a partial class, having one file for MyClass, other for MyClassConditions and other for MyClassProcessExecution.

Defining them as internal?

I don't really like that much of the internal modifier, as I don't find these classes add any value at all for the rest of my program/assembly, and I'd like to hide them if possible. It's not like they're gonna be useful/reusable in any other part of the program.

Keep them as public?

I can't see why, but I've let this option here.

Any other?

Name it!

Thanks

Answer 1

For "Helper" type classes that aren't going to be used outside the current assembly, Internal is the way to go if the methods are going to be used by multiple classes.

For methods that are only going to be used by a single class, I'd just make them private to the class, or use inner classes if it's actually a class that's not used anywhere else. You can also factor out code into static methods if the code doesn't rely on any (non-static) members of your class.

Answer 2

Your best bet is probably to use partial classes and put the three clumps of code in separate files adding to the same class. You can then make the conditional and process code private so that only the class itself can access them.

Answer 3

Create the classes with the same access modifier as the methods you have refactored. Partial classes are only really usefull when you have multiple people or automat5ed code generating tools frequently modifying the same classes. They just really avoid source merge hell where your source controll mashes your code because it can't merge multiple edits to the same file.

Answer 4

I can define MyClass as a partial class, having one file for MyClass, other for MyClassConditions and other for MyClassProcessExecution.

Maybe it's my C++ background, but this is my standard approach, though I bundle small helper classes together into a single file.

Thus, on one of my current projects, the Product class is split between Product.cs and ProductPrivate.cs

Answer 5

I'm going for something else - the issue of public / protected / private may not be solved specifically by this, but I think it lends itself much better to maintenance then a lot of nested, internal classes.

Since it sounds like you've got a set of steps in a sequential algorithm, where the execution of one step may or may not be dependent upon the execution of the previous step. This type of sequential step processing can sometimes use the Chain of Responsibility Pattern, although it is morphed a little bit from its original intention. Focussing only on your "processing method", for example, starting from something like below:

class LargeClass
{
public void DoProcess()
{
  if (DoProcess1())
  {
    if (DoProcess2())
    {
      DoProcess3();
    }
  }
}

protected bool DoProcess1()
{
...
}

protected bool DoProcess2()
{
...
}

protected bool DoProcess3()
{
...
}

}

Using Chain of Responsibility, this could be decomposed into a set of concrete classes for each step, which inherit from some abstract step class. The abstract step class is more responsible for making sure that the next step is called, if the necessary preconditions are met.

public class AbstractStep
{
    public AbstractStep NextStep { get; set; }

    public virtual bool ExecuteStep
    {
       if (NextStep != null)
       {
         return NextStep.ExecuteStep();
       }
    }  
}

public class ConcreteStep1 : AbstractStep
{
    public bool ExecuteStep
    {
       // execute DoProcess1 stuff
       // call base
       return base.ExecuteStep();
    }
}

...

public class ConcreteStep3 : AbstractStep
{
     public bool ExecuteStep
     { 
        // Execute DoProcess3 stuff
        // call base
        return true; // or false?
      }
}

To set this up, you would, in some portion of the code, do the following:

var stepOne = new ConcreteStep1();
var stepTwo = new ConcreteStep2();
var stepThree = new ConcreteStep3();
stepOne.NextStep = stepTwo;
stepTwo.NextStep = stepThree;

bool success = stepOne.ExecuteStep();

This may help clean up the code bloat you've got in your single class - I've used it for a few sequential type algorithms in the past and its helped isolate each step nicely. You could obviously apply the same idea to your condition checking (or build them into each step, if that applies). You can also do some variation on this in terms of passing state between the steps by having the ExecuteStep method take a parameter with a state object of some sort.

Of course, if what you're really concerned about in this post is simply hiding the various steps, then yes, you could make each of your substeps a protected class within your class that creates the steps. Unless you're exposing your library to customers in some form or fashion however, and you don't want them to have any type of visibility into your execution steps, this seems to be a smaller concern then making the code maintainable.