Creating clearly defined methods with generics and delegates

Question

I'm fairly new to OOP and have been working heavily with C# lately. I've come across an issue in which I had a lot of duplicate code with only a query and how to handle the query results changing.

I significantly cut down on the code by creating a static class that accepts generics and an action delegate but it has a ton of arguments required and I'm looking for a cleaner way to accomplish this.

Here's how it's currently working:

public static void RunReport<TIcketType, TReportType>(Guid reportId, DateTime startDate, DateTime endDate, string queryString, Action<TIcketType, TReportType> parser) 
    where TIcketType : Medcare.Core.models.APPOC.BaseTicket
    where TReportType : new()
{
    var reportProcessFilename = System.IO.Path.GetTempPath() + "Report_" + reportId + ".work";
    var reportJsonFilename = System.IO.Path.GetTempPath() + "Report_" + reportId + ".JSON";
    try
    {
        // As long as this file exists we are assumed to be working on the report
        using (System.IO.File.Create(reportProcessFilename)){}

        var finishedReport = new TReportType();
        using (var docStore = new DocumentStore { Url = "http://MyRavenDB.com", DefaultDatabase = "MyRavenDB" })
        {
            docStore.Initialize();

            using (var session = docStore.OpenSession())
            {
                var query = session.Advanced.LuceneQuery<TIcketType>("Ticket/Search")
                    .Where(string.Format(queryString, startDate, endDate));

                QueryHeaderInformation qhi;
                var stream = session.Advanced.Stream(query, out qhi);

                while (stream.MoveNext())
                {
                    var ticket = stream.Current.Document;
                    ticket.id = stream.Current.Key;
                    parser(ticket, finishedReport);
                }
            }
        }
        var json = JsonConvert.SerializeObject(finishedReport);
        System.IO.File.WriteAllText(reportJsonFilename, json);
    }
    catch (Exception ex)
    {
        var json = "{success: false, message: " + ex.Message + "}";
        System.IO.File.WriteAllText(reportJsonFilename, json);
    }
    finally
    {
        if (System.IO.File.Exists(reportProcessFilename))
            System.IO.File.Delete(reportProcessFilename);
    }
}

I have to pass a generic to the ravenDB function "LuceneQuery", it attempts to dynamically fill out the query based on the generic that was passed to it. It must inherit our TicketBase class so I can be sure the generic passed contains some base properties.

Here's the code that calls this function from an API route:

public static ReportRequestResponse RunReport<TRequest>(TRequest request, ReportTypes reportType)
    where TRequest : ReportRequest
{
    if (!string.IsNullOrEmpty(request.ReportId))
    {
        var targetReportWorkingFilename = System.IO.Path.GetTempPath() + "Report_" + request.ReportId + ".work";
        var targetReportFilename = System.IO.Path.GetTempPath() + "Report_" + request.ReportId + ".JSON";

        if (!System.IO.File.Exists(targetReportFilename))
            return new ReportRequestResponse
            {
                ReportId = request.ReportId,
                Report = "Report ID not found",
                Status = "failure"
            };

        string reportResults;
        string status;
        if (System.IO.File.Exists(targetReportWorkingFilename))
        {
            status = "working";
            reportResults = "Waiting on Report to Finish";
        }
        else
        {
            status = "done";
            using (var sr = new System.IO.StreamReader(targetReportFilename))
                reportResults = sr.ReadToEnd();
        }

        return new ReportRequestResponse
        {
            ReportId = request.ReportId,
            Report = reportResults,
            Status = status
        };

    }
    if (!request.StartDate.HasValue || !request.EndDate.HasValue)
        return new ReportRequestResponse
        {
            ReportId = string.Empty,
            Status = "failure",
            Report = "StartDate and EndDate are required."
        };

    // Starting a new Report Thread
    var newReportId = Classes.Reports.ReportsHelper.GenerateNewReportID(); // Generate a valid GUID that doesn't exist yet

    var reportFilename = System.IO.Path.GetTempPath() + "Report_" + newReportId + ".JSON";
    if (System.IO.File.Exists(reportFilename)) System.IO.File.Delete(reportFilename);
    using (System.IO.File.Create(reportFilename)) { }

    switch (reportType)
    {
        case ReportTypes.ByteSuccess:
            Classes.APPOC.ByteSuccessReport.Start(newReportId, request.StartDate.Value, request.EndDate.Value);
            break;
        case ReportTypes.DocRequest:
            Classes.APPOC.DocumentRequestByType.Start(newReportId, request.StartDate.Value, request.EndDate.Value);
            break;
        case ReportTypes.TherapySales:
            Classes.APPOC.TherapySales.Start(newReportId, request.StartDate.Value, request.EndDate.Value);
            break;
    }

    return new ReportRequestResponse
    {
        ReportId = newReportId.ToString(),
        StartDate = request.StartDate,
        EndDate = request.EndDate,
        Status = "running",
        Report = reportFilename
    };
}

}

The logic:

    /* LOGIC:
     * If date range is specified-
     *    Generate new report GUID and call desired RunReport depending on ReportType
     *    HTTP Response with report GUID
     * If report ID is specified
     *    Look for report JSON file
     *      If found check for a .work file
     *          If found http response with status: working
     *          If not found http response with GUID.JSON file
     *      If not found http response with no report with that GUID exists
    */

And here's a specific Report class calling the function:

private static void RunReport(Guid reportId, DateTime startDate, DateTime endDate)
{
    const string queryString = "type: 8?? AND dateCreated: [{0:yyyy-MM-dd} TO {1:yyyy-MM-dd}]";
    ReportRunner.RunReport<DocRequestFullNotes, Dictionary<string, TicketSummary>>
        (reportId, startDate, endDate, queryString,
        (ticket, finishedReport) =>
        {
            var therapies = Medcare.Core.models.APPOC.Therapies.therapies;
            var targetTherapy = therapies.FirstOrDefault(t => t.startingTicketType == ticket.type);
            var myTicket = new Ticket
            {
                Status = ticket.status,
                Id = ticket.id,
                CreatedBy = ticket.createdBy,
                DateCreated = ticket.dateCreated,
                SourceId = ticket.source.id,
                Details = ticket.details,
                Path = ticket.CurrentPath,
                Notes = ticket.notes
            };
            if (targetTherapy != null && !finishedReport.ContainsKey(targetTherapy.id))
            {
                var therapy = new TicketSummary {Name = targetTherapy.name};
                finishedReport.Add(targetTherapy.id, therapy);
            }

            if (targetTherapy != null && !finishedReport.ContainsKey(targetTherapy.id))
            {
                var therapy = new TicketSummary {Name = targetTherapy.name};
                finishedReport.Add(targetTherapy.id, therapy);
            }

            if (targetTherapy == null) return;
            finishedReport[targetTherapy.id].AddTicket(myTicket);
        });
}

Answer 1

I think that when a method has too many parameters then there are good chances that it is doing too many things. Count them:

• Create a lock-file.
• Query the document store.
• Read data from the document store.
• Filter data.
• Invoking the parsing function.
• Save the report.
• Clean-up.

Starting from the top what I'd ideally like to write is something similar to this (almost pseudo-code), which can be tested without any interaction with the file system:

var tickets = query.Select(x => {
    var ticket x.Document;
    ticket.id = x.Key;
    parser(ticket, finishedReport);
});

That's the central function, let's call it ParseTickets(), you want to test in isolation. Everything else should be built around it:

var serializedTickets = converter.SerializeObject(ParsesTickets(query));
File.WriteAllText(outputPath, serializedTickets, Encoding.UTF8);

You can see I introduced a separate object for serialization, I can now serialize with JSON but it might be YAML or XML in future. Now let's introduce the lock file:

using (new LockFile(lockFilePath))
{
    var serializedTickets = converter.SerializeObject(ParsesTickets(query));
    File.WriteAllText(outputPath, serializedTickets, Encoding.UTF8);
}

Note that in this way you do not reduce the total number of inputs you need to perform a specific task (it's obviously impossible unless you have some duplication) but you group them in logical entities. One single parameter Query contains the logic to connect to the document store and to query values you need. Query itself does not know how to connect to the document store, it delegates to a Connection object. In this way you can test connection, query and parsing in separation.

Let's imagine the calling point:

var query = Query.FromDateRange(connection, startDate, endDate);
RunReport<TicketType, ReportType>(reportId, query, parser);

I think you understood the point.

You may want to promote RunReport as instance member of a concrete class Report. It might be named, for example, Build() and instance properties (such has Query, Parser, OutputPath and so on) will make your code much more clear.

---

Few minor issues.

using (System.IO.File.Create(reportProcessFilename)){}

This has been replaced by an appropriate class which explain the intent of that code (no need to write comments) and also handles the logic to delete the file when we're done. Note, however, that to create an empty file you may simply use File.WriteAllText(path, "").

You check if file exists before you delete it but...it's not safe. File might be removed after File.Exists() your check and before File.Delete(), wrap that call into a try/catch (catching only IOException). Even better wait and retry in case of errors. Note that if you keep the file open in LockFile without any sharing then no one will be able to delete the file while it's in use.

You're catching Exception but I suppose to return an error message isn't the best way to handle, for example, an OutOfMemoryException or an AccessViolationException. Catch exactly what you expect and/or you can handle. Anything else is a bug in your code and it must be fixed, not silently ignored. Not to mention that it might leak embarrassing or confidential information to end-user.

Don't manually build paths, use Path.Combine(). Also you can use string interpolation feature. An hypothetical LockFile.GetPath() function might be:

static string GetPath(Guid reportId)
    => Path.Combine(Path.GetTempPath(), $"Report_{reportId}.work";

I don't know your application logic but...why do you use temp path to save reports? Also you do not return the full file path to the caller...do you repeat that logic at calling point (build full path from reportId)? If it's the case then do not do it, if you need this information then return it.

Answer 2

     * If report ID is specified
     *    Look for report JSON file
     *      If found check for a .work file
     *          If found http response with status: working
     *          If not found http response with GUID.JSON file
     *      If not found http response with no report with that GUID exists

This looks backwards to me. RunReports creates the lock file, then creates the JSON file, and finally deletes the lock file. I think the logic should be

     * If report ID is specified
     *    Check for a .work file
     *       If found http response with status: working
     *    Look for report JSON file
     *       If found http response with GUID.JSON file
     *       If not found http response with no report with that GUID exists

---

The lifecycle of the lock file also looks slightly wrong:

    try
    {
        // As long as this file exists we are assumed to be working on the report
        using (System.IO.File.Create(reportProcessFilename)){}

        ...
    }
    catch (Exception ex)
    {
        var json = "{success: false, message: " + ex.Message + "}";
        System.IO.File.WriteAllText(reportJsonFilename, json);
    }
    finally
    {
        if (System.IO.File.Exists(reportProcessFilename))
            System.IO.File.Delete(reportProcessFilename);
    }

There are three possibilities:

1. (Expected execution) The file is created in the try block and so exists to be deleted in the finally block.
2. The try block fails to create the file and it doesn't exist in the finally block. The most likely explanation would be that the process doesn't have write permissions for %TEMP%.
3. The try block fails to create the file because some other process has it open. It might then hit the finally block and try to delete a file it didn't create.

I'm not entirely convinced by the decision to use lock files. Is there documentation explaining why temporary files are used instead of a database (e.g. Lucene, since it's obviously present!)?

---

                while (stream.MoveNext())
                {
                    var ticket = stream.Current.Document;
                    ticket.id = stream.Current.Key;
                    parser(ticket, finishedReport);
                }

I'm not sure what the type of stream is. Does it support for (var keyDocPair in stream)?

---

TIcketType

Is this intended to be TTicketType or TicketType?

---

        var json = "{success: false, message: " + ex.Message + "}";
        System.IO.File.WriteAllText(reportJsonFilename, json);

Aside from Adriano Repetti's comment about exposing internal information, there are various potential sources of exceptions which you don't control. I would be surprised if any of them produce an ex.Message which is a valid JSON value. And just in case you're thinking of changing the code to

        var json = "{success: false, message: \"" + ex.Message + "\"}";

what guarantees are there that ex.Message won't contain an unescaped "?