4
\$\begingroup\$

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);
        });
}
\$\endgroup\$
4
\$\begingroup\$

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.

\$\endgroup\$
3
  • \$\begingroup\$ Thanks for all the advice. The reports are being saved in the temp path because they aren't meant to be kept, they are generated on demand for a web application where the user can save the report manually if desired, otherwise it gets cleared out at the end of the day. \$\endgroup\$
    – Brian Ge
    Jul 6 '17 at 13:16
  • \$\begingroup\$ I meant to say it always gets cleared out from our server at the end of the day, it's up to them to save reports. The paths are used in an API route that is accessed from the web application. The user selects a date range, the api route generates a GUID, calls RunReport, and responds with the report ID. The web app calls the route with the report ID. The route checks for the report .work and .json files and responds accordingly and the web app will retry until it's done or hits max retries. \$\endgroup\$
    – Brian Ge
    Jul 6 '17 at 13:32
  • \$\begingroup\$ I understand, I think it may be better to do not hard-code this convention. If the two actions are unrelated then you should expose a (static) method Report.GetReportPathFromId(Guid reportId) instead of repeat that logic inside RunReport() and inside web app. It's a small thing but it helps to avoid changes in multiple places when logic is updated (for example if you decide to move temp reports to a different path or to split them in different directories with different cleaning rules, just to say) \$\endgroup\$ Jul 6 '17 at 13:58
2
\$\begingroup\$
     * 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 "?

\$\endgroup\$
3
  • \$\begingroup\$ Thanks for the advice. Using the database instead of a temporary file is a great idea however I don't have permission to create new tables in the database. I'm not sure I'm using lock files as intended, in fact the whole web client implementation is kind of hacky if you ask me. The web client sends a report request to an API route, the API responds with the report ID, the client waits 5 seconds and then sends a request to the route with the report ID, the route checks for the work file and responds with either working or the report, if "working" the client repeats the request every 5 secs. \$\endgroup\$
    – Brian Ge
    Jul 10 '17 at 18:55
  • \$\begingroup\$ So I looked more into lockfiles, specifically pessimetic and optimistic offline locks. My issue doesn't seem to warrant a lockfile since it's not an issue of multiple user accessing and modifying the same data. My issue is the report generation can take a very long time and I need a way to indicate it's still being worked on. \$\endgroup\$
    – Brian Ge
    Jul 10 '17 at 19:16
  • \$\begingroup\$ So I ended up significantly refactoring this segment of code, instead of a work file I am now using a Redis cache from servicestack. Now it's thread safe and free of any possible IO exceptions. \$\endgroup\$
    – Brian Ge
    Jul 19 '17 at 13:37

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.