How do I make this code more readable?

Question

I wrote this today and I'm ashamed. What do I need to do to make this chaotic stuff more accurate and readable amongst others?

switch ((RequestReportsCalculatingStoredProcedures.RequestReportStoredProcedureType)Enum.Parse(typeof(RequestReportsCalculatingStoredProcedures.RequestReportStoredProcedureType), ihdType.Value))
    {
            //REF:This can (but should it?) be refactored through strategy pattern
        case RequestReportsCalculatingStoredProcedures.RequestReportStoredProcedureType.ReportPlanWithEffects:
            grvEconomicCriteria.DataSource = RequestReportsCalculatingStoredProcedures.ReportsDataParser(
                            RequestReportsCalculatingStoredProcedures.ReportPlanWithEffects(requestNo, RequestReportsCalculatingStoredProcedures.GetAlgorithmNoByRequestNo(requestNo)));
            break;
        case RequestReportsCalculatingStoredProcedures.RequestReportStoredProcedureType.ReportPlanWithEffectsForFacts:
            DateTime factDate;
            try
            {
                factDate = Convert.ToDateTime(ihdDate.Value);
            }
            catch(FormatException)
            {
                grvEconomicCriteria.DataSource = RequestReportsCalculatingStoredProcedures.ReportsDataParser(
                            RequestReportsCalculatingStoredProcedures.ReportPlanWithEffectsForFacts(requestNo, RequestReportsCalculatingStoredProcedures.GetAlgorithmNoByRequestNo(requestNo), DateTime.MinValue));
                break;
            }
            grvEconomicCriteria.DataSource = RequestReportsCalculatingStoredProcedures.ReportsDataParser(
                            RequestReportsCalculatingStoredProcedures.ReportPlanWithEffectsForFacts(requestNo, RequestReportsCalculatingStoredProcedures.GetAlgorithmNoByRequestNo(requestNo), factDate));
            break;
        default:
            break;
    }

Answer 1

You can always use an alias for the very long type RequestReportsCalculatingStoredProcedures:

using RRCSP = RequestReportsCalculatingStoredProcedures;

Note: You will need to use the fully qualified name (including namespace) in the using directive.

Answer 2

Have a look at the using command.

Also I beleive (haven't checked) but you can do something similar to:

RequestReportsCalculatingStoredProcedure myShortCut = RequestReportsCalculatingStoredProcedures.RequestReportStoredProcedureType;

Then instead of referencing the large lines you can reference myShortCut.

Answer 3

You could apply the strategy pattern as you commented, or use a Dictionary with delegates(Func<> or Action<>) that based on a type does something. You can shorten the DateTime try/catch by using DateTime.TryParse() method. Other than that, what is most unredeable from your code is the long names, so you may want to put some of those in variable before starting your code

hope it helps

Answer 4

I am a strong believer in descriptive variable, class and method names. I would always opt for clarity over brevity, however one easy to implement rule is that if you have a part of your names that always repeats, you can pare that out. For example, you have:

RequestReportsCalculatingStoredProcedures

That's good. It explains what it is clearly. However you've got members of that class or object that also start with that name plus differentiating names at the end. For example:

RequestReportStoredProcedureType

This could be shortened to StoredProcedureType, or even arguably ProcedureType.

I know many programmers would argue for something like RRCalcSP or some other completely unclear naming conventions, but I would never sacrifice naming clarity for avoiding line wrap, etc.

Honestly what you have in your original example is not chaotic or shameful. It's just so long that you have to deal with line wrap.

Also, generous use of comments make things much clearer.

Answer 5

If you have some control over the code that you're using, I'd recommend:

1. Bring the enum RequestReportsCalculatingStoredProcedures.RequestReportStoredProcedureType to outer scope. The inner name is already verbose enough that you don't need the outer name to clarify.

2. Try to replace the long class names with names that clarify their intent, without describing the details of how they accomplish their job.

3. Provide an overload of RequestReportsCalculatingStoredProcedures.ReportPlanWithEffectsForFacts that doesn't require the algorithm parameter, since you can apparently look it up by the requestNo anyway. That eliminates the verbose call to RequestReportsCalculatingStoredProcedures.GetAlgorithmNoByRequestNo when you don't need to provide a non-standard algorithm.

Answer 6

Honestly, one of the biggest issues here is just the length of your variable names.

Obviously, you should give variables/types/etc. descriptive names. But there's a point at which it gets a bit extreme. One guy at my work is notorious for giving methods names like:

DoSomethingVerySpecificHereIsOneOfItsSideEffectsAndHereIsAnother

In your case, I notice a great deal of redundancy. For example, you have a class called RequestReportsCalculatingStoredProcedures, and then within that class you seem to have an enum called RequestReportStoredProcedureType. Since the enum is already a nested type within RequestReportsCalculatingStoredProcedures, maybe you could just call it Type?

Alternatively, a very effective way to shorten these names (at least in this file) would be to use a simple using declaration:

using E = RequestReportsCalculatingStoredProcedures.RequestReportStoredProcedureType;

Then look what happens to your code:

using RRCSP = RequestReportsCalculatingStoredProcedures;
using E = RRCSP.RequestReportStoredProcedureType;

// ...

// Note: RRCSP = RequestReportsCalculatingStoredProcedures, and
//       E = RRCSP.RequestReportStoredProcedureType

switch ((E)Enum.Parse(typeof(E), ihdType.Value))
    {
        //REF:This can (but should it?) be refactored through strategy pattern
        case E.ReportPlanWithEffects:
            grvEconomicCriteria.DataSource = RRCSP.ReportsDataParser(
                RRCSP.ReportPlanWithEffects(
                    requestNo,
                    RRCSP.GetAlgorithmNoByRequestNo(requestNo)
                )
            );
            break;
        case E.ReportPlanWithEffectsForFacts:
            DateTime factDate;
            try
            {
                factDate = Convert.ToDateTime(ihdDate.Value);
            }
            catch(FormatException)
            {
                grvEconomicCriteria.DataSource = RRCSP.ReportsDataParser(
                    RRCSP.ReportPlanWithEffectsForFacts(
                        requestNo,
                        RRCSP.GetAlgorithmNoByRequestNo(requestNo),
                        DateTime.MinValue
                    )
                );
                break;
            }
            grvEconomicCriteria.DataSource = RRCSP.ReportsDataParser(
                RRCSP.ReportPlanWithEffectsForFacts(
                    requestNo,
                    RRCSP.GetAlgorithmNoByRequestNo(requestNo),
                    factDate
                )
            );
            break;
        default:
            break;
    }

Is this more readable? In my opinion, yes, primarily because it's simply less daunting to look at. Obviously you're making a trade-off, though, as the aliased names you're using are less immediately obvious than the original names. Like just about everything else, it ultimately comes down to personal judgment.

Answer 7

On top of what the others have said about shortening the names etc, you should think about extracting the code from the case statement into function calls, so you end up with something like

switch (myMassiveVariable)
{
    case RequestReportStoredProcedureType.ReportPlanWithEffects:
        RunReportWithEffects(requestNo);
        break;
    case RequestReportStoredProcedureType.ReportPlanWithEffectsForFacts:
        RunReportWithFacts(requestNo);
        break;
}

That helps to tidy things up a bit.

Answer 8

i'd advocate extracting the common parts out of the case statement

RequestReportsCalculatingStoredProcedures.RequestReportStoredProcedureType dependingOnType =(RequestReportsCalculatingStoredProcedures.RequestReportStoredProcedureType)Enum.Parse(typeof(RequestReportsCalculatingStoredProcedures.RequestReportStoredProcedureType), ihdType.Value);

var EssentialData = null;
var AlgorithmChosen = RequestReportsCalculatingStoredProcedures.GetAlgorithmNoByRequestNo(requestNo);

switch (dependingOnType)
    {
            //REF:This can (but should it?) be refactored through strategy pattern
        case RequestReportsCalculatingStoredProcedures.RequestReportStoredProcedureType.ReportPlanWithEffects:
           EssentialData  = RequestReportsCalculatingStoredProcedures.ReportPlanWithEffects(requestNo, AlgorithmChosen);
            break;
        case RequestReportsCalculatingStoredProcedures.RequestReportStoredProcedureType.ReportPlanWithEffectsForFacts:
            DateTime factDate = Datetime.MinValue;

            if (!DateTime.TryParse(ihdDate.Value, factDate)) {
                factDate = DateTime.MinValue;
            }

               EssentialData  = RequestReportsCalculatingStoredProcedures.ReportPlanWithEffectsForFacts(requestNo, AlgorithmChosen, factDate);

            break;
        default:
            break;
    }

grvEconomicCriteria.DataSource = RequestReportsCalculatingStoredProcedures.ReportsDataParser(essentialData);

You could refine it event more by calculating the

RequestReportsCalculatingStoredProcedures.GetAlgorithmNoByRequestNo(requestNo)

only once

edit: like this :)

Answer 9

Admittedly this answer is leveraging on other answers to this question, but I thought it would be helpful to make some of the suggestions more explicit.

The class RequestReportsCalculatingStoredProcedure I would rename to RequestReports. The StoredProcedure portion to me seems to be an implementation detail that is nobody else's business. I am not sure what the word Calculating brings to the table so I removed that also.

The enumeration RequestReportStoredProcedureType I would rename to ReportPlan as that seemed to fit the context best (ReportType is also a possibility). The additional wordiness was removed similar to the reasons the class that encompasses it was renamed. I left it inside the class as it seems to provide some context, and with the names shortened this seemed like a good idea.

The suggestions to remove the algorithm parameter from the method calls since it can be derived from an already passed parameter seemed like a good one.

Given these changes the code would then look like the following:

switch((RequestReports.ReportPlan)Enum.Parse(typeof(RequestReports.ReportPlan), ihdType.Value)) {
            //REF:This can (but should it?) be refactored through strategy pattern
            case RequestReports.ReportPlan.WithEffects:
                Object reportPlan = RequestReports.ReportPlanWithEffects(requestNo);
                grvEconomicCriteria.DataSource = RequestReports.ReportsDataParser(reportPlan);
                break;
            case RequestReports.ReportPlan.WithEffectsForFacts:
                DateTime factDate;
                try {
                    factDate = Convert.ToDateTime(ihdDate.Value);
                }
                catch(FormatException) {
                    Object reportPlan2 = RequestReports.ReportPlanWithEffectsForFacts(requestNo, DateTime.MinValue);
                    grvEconomicCriteria.DataSource = RequestReports.ReportsDataParser(reportPlan2);
                    break;
                }
                Object reportPlan3 = RequestReports.ReportPlanWithEffectsForFacts(requestNo, factDate);
                grvEconomicCriteria.DataSource = RequestReports.ReportsDataParser(reportPlan3);
                break;
            default:
                break;
            }