Take the 2-minute tour ×
Programmers Stack Exchange is a question and answer site for professional programmers interested in conceptual questions about software development. It's 100% free, no registration required.

I've been working on some code recently that interfaces with a CMS we use and it's presented me with a question on class design that I think is applicable in a number of situations. Essentially, what I am doing is extracting information from the CMS and transforming this information into objects that I can use programatically for other purposes. This consists of two steps:

  1. Retrieve the data from the CMS (we have a DAL that I use, so this is essentially just specifying what data from the CMS I want--no connection logic or anything like that)
  2. Map the parsed data to my own [C#] objects

There are basically two ways I can approach this:

One call from multiple methods

public void MainMethodWhereIDoStuff()
{
    IEnumerable<MyObject> myObjects = GetMyObjects();
    // Do other stuff with myObjects
}

private static IEnumerable<MyObject> GetMyObjects()
{
    IEnumerable<CmsDataItem> cmsDataItems = GetCmsDataItems();
    List<MyObject> mappedObjects = new List<MyObject>();
    // do stuff to map the CmsDataItems to MyObjects
    return mappedObjects;
}

private static IEnumerable<CmsDataItem> GetCmsDataItems()
{
    List<CmsDataItem> cmsDataItems = new List<CmsDataItem>();
    // do stuff to get the CmsDataItems I want
    return cmsDataItems;
}

Multiple calls from one method

public void MainMethodWhereIDoStuff()
{
    IEnumerable<CmsDataItem> cmsDataItems = GetCmsDataItems();
    IEnumerable<MyObject> myObjects = GetMyObjects(cmsDataItems);
    // do stuff with myObjects
}

private static IEnumerable<MyObject> GetMyObjects(IEnumerable<CmsDataItem> itemsToMap)
{
    // ...
}

private static IEnumerable<CmsDataItem> GetCmsDataItems()
{
    // ...
}

I am tempted to say that the latter is better than the former, as GetMyObjects does not depend on GetCmsDataItems, and it is explicit in the calling method the steps that are executed to retrieve the objects (I'm concerned that the first approach is kind of an object-oriented version of spaghetti code).

On the other hand, the two helper methods are never going to be used outside of the class, so I'm not sure if it really matters whether one depends on the other. Furthermore, I like the fact that in the first approach the objects can be retrieved from one line-- most likely anyone working with the main method doesn't care how the objects are retrieved, they just need to retrieve the objects, and the "daisy chained" helper methods hide the exact steps needed to retrieve them (in practice, I actually have a few more methods but am still able to retrieve the object collection I want in one line).

Is one of these methods right and the other wrong? Or is it simply a matter of preference or context dependent?

share|improve this question
    
Not an answer but a simple tip. If you're going to create a class that extends one of the generic collection classes add the IEnumerable interface to your class and take some time to implement the required methods. That way you'll be able to iterate over your custom collection in a standard/predictable fashion (ex using foreach). –  Evan Plaice Oct 21 '13 at 20:52
add comment

6 Answers

up vote 3 down vote accepted

The key is "are never going to be used outside of the class"

Structure doesn't matter when the implementation remains private. You're free to change/modify it however you want because nobody will depend on the structure.

Public members/interfaces OTOH, do matter. When you expose a member publically/internally, you should have an explicit reason for doing so because somebody may/will depend on it in the future. That's the dividing line between application and API development. In APIs access control becomes very important because any member/interface that people depend on is one that can't be changed/removed without breaking somebody's code.

In essence:

your private parts are nobody's business but your own

share|improve this answer
4  
Technically in C++ friends can still touch your privates (but though probably shouldn't) –  Ben Brocka May 2 '12 at 16:27
    
@BenBrocka I've heard that, sounds risqué. –  Evan Plaice May 2 '12 at 16:28
    
Popular opinion considers it rather inappropriate –  Ben Brocka May 2 '12 at 16:35
    
When GetCmsDataItems() is and will be a method only used by GetMyObjects(..) then I think it should be called by GetMyObjects. –  Lucas Oct 18 '13 at 10:51
    
@Lucas The names used are purely subjective. The point is, the helper methods will never be exposed publicly -- as in they will never be used outside the class definition -- so the naming scheme only needs to make sense to the developer(s) implementing the class. The simple rule being, don't obsess too much over naming/structure when it comes to code private/helper code. –  Evan Plaice Oct 21 '13 at 20:14
add comment

The former example you give more thoroughly insulates the "main" code section (which I assume sees the most developer traffic) from side-concerns, so I believe that is the better approach. However, like you, I don't like having a lot of helper methods clogging up my main classes. To make that a little cleaner, consider separating the helper methods into a helper class. You could do this with a separate internal class or if you want the class private for encapsulation concerns, create a partial class file and make your helper a private nested class. Like so:

// helper class definition file
public partial class MyClass
{
    // private class modifier only works on nested classes like this one
    // only the parent class MyClass will be able to see this class
    private static class MyHelper
    {
        public static IEnumerable<MyObject> GetMyObjects() { ... }
        private static IEnumerable<CmsDataItem> GetCmsDataItems() { ... }
    }
}

// normal, high traffic file
public partial class MyClass
{
    public void MainMethodWhereIDoStuff()
    {
        IEnumerable<MyObject> myObjects = MyHelper.GetMyObjects();
    }

    // removed side-concerns of getting / transforming data into MyObjects
}

Something like this isn't strictly necessary, but may help clarify concerns if that is important to you.

As a side note, for your data transformations (e.g. from CmsDataItem to MyObject), you might consider using a mapping library like AutoMapper (which is available through NuGet in Visual Studio). Although you will still have to maintain mapping transformations, it will make your code semantically more clear that you are just mapping properties from one object to another. Plus AutoMapper will do a lot of simpler mappings automatically.

share|improve this answer
add comment

The first example is what is typically called a Method Chain. There isn't really anything inherently wrong with method chains per-se, however long chains may be difficult to manage and understand, even while they might make the code appear cleaner and clearer to read. If necessary, then sometimes you just have to put up with them, however it can sometimes be preferable to replace a method chain with a helper method or class that handles all of the chained calls for you, particularly if you find yourself needing to use a particular chain often.

The second example is fine for relatively small methods that do not need to do more than a few things. As the number of calls increases however, the code often becomes less clear and more difficult to read. Again, there is nothing inherently bad with having a method with multiple calls, however the risk is that for some operations you might find code being duplicated. If the method grows beyond a handful of lines, chances are there might be other methods and possibly even a class waiting to be discovered.

What is important, is to consider whether the technique you use is appropriate to the task involved, and whether it will have an impact on the interface of the class that the method belongs to. While the immediate goal is to satisfy a requirement, other goals may also include extensibility and low maintenance overhead. Experience with your particular applications will teach you when to favour one technique over another.

share|improve this answer
add comment

I think you should look it in more of a maintainability and code-correctness point of view.

For example, in the first case getMyObjects and getCMSObjects are coupled so that the only objects that getMyObjects will ever be able to map are the objects freshly returned from getCmsObjects, while on the second example the method operates on whatever argument it receives.

If your implementation of getMyObjects is flexible enough to allow for any list of objects as input, the you should err for the second example, since it is more flexible and doesn't cost very much. However, if the logic in getMyObjects only works on a list of freshly returned cms objects, putting a parameter will only serve as a place to let you introduce errors (by passing the wrong parameter) so you should prefer the parameter-less version instead.

share|improve this answer
    
Thanks for the input.. I did think about it in terms of coupling too, but then I thought I might be getting a little out of control talking about decoupling private methods in a class. –  Andrew May 3 '12 at 22:32
add comment

I don't think either approach is right or wrong. It's just a matter of preference. As you said, both methods are private and so it won't affect external usability. What's important is that it's clear what is necessary in order to accomplish the necessary task and it seems that both approaches are clear.

share|improve this answer
add comment
  • It'll be practically impossible give you the "right answer" until you rename your methods and give them a better semantic meaning rather than just "DoStuff".

  • Only then we will be able to to identify responsibilities and separate them respectively.

share|improve this answer
add comment

Your Answer

 
discard

By posting your answer, you agree to the privacy policy and terms of service.

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