6
\$\begingroup\$

I'm building a tool that takes in web requests and deserializes JSON requests into a C# class. Each page calls a different controller URL that is looking for a specific request. Once the request is received, I pipe the request into a method that iterates over each property in the class using reflection looking for the presence of a certain attribute. If the attribute is present, it reads the PDF field name from that attribute and the application then uses ITextSharp to populate a PDF form.

My solution received many pats on the back from my coworkers, but I still feel like it can be done better, in a much more decoupled way. My initial idea was to create an interface that each class implements, and then use custom methods inside each class to map out the properties manually. However, this seems a bit more cumbersome and involved.

Does anyone have any input on what a better practice might be?

PDF generation method:

public static byte[] GeneratePDF(string basepath, string filepath, object response)
{
    var pdfTemplate = filepath;
    var pdfReader = new PdfReader(pdfTemplate);
    var newPdf = new MemoryStream();
    var pdfStamper = new PdfStamper(pdfReader, newPdf);

    try
    {
        var pdfFormFields = pdfStamper.AcroFields;
        pdfFormFields.SetField("Date", DateTime.Now.ToString("MM/dd/yyyy"));
        var properties = response.GetType().GetProperties();
        foreach (PropertyInfo property in properties)
        {
            var attr = (PDFFieldName)Attribute.GetCustomAttribute(property, typeof(PDFFieldName));
            if (attr != null)
            {
                var prop = property.GetValue(response)?.ToString() ?? "";
                if (prop.ToLower() == "true") prop = "On";
                pdfFormFields.SetField(attr.Name, prop);
            }
        }

        pdfStamper.Close();
        newPdf.Flush();

        var finalPdf = newPdf.ToArray();
        var signature = response.GetType().GetProperty("signature");

        if (signature != null)
        {
            var signer = new PDFSigner();
            var signatureString = response.GetType()?.GetProperty(nameof(signature))?.GetValue(response, null)?.ToString() ?? "";

            finalPdf = signer.SignPDF(basepath, finalPdf, signatureString, "Paperwork");
        }


        return finalPdf;
    }
    catch (Exception ex)
    {
        Console.WriteLine(ex);
    }
    finally
    {
        pdfStamper.Dispose();
        pdfReader.Close();
        pdfReader.Dispose();
        newPdf.Close();
        newPdf.Dispose();
    }

    return null;
}

Example PDF class:

public class ShareMedicalInfoResponse
{
    [PDFFieldName("Name1")]
    public string name1 { get; set; }
    [PDFFieldName("Name2")]
    public string name2 { get; set; }
    [PDFFieldName("Name3")]
    public string name3 { get; set; }
    [PDFFieldName("Name4")]
    public string name4 { get; set; }
    [PDFFieldName("Name5")]
    public string name5 { get; set; }
    [PDFFieldName("Name6")]
    public string name6 { get; set; }
    [PDFFieldName("Relationship1")]
    public string relationship1 { get; set; }
    [PDFFieldName("Relationship2")]
    public string relationship2 { get; set; }
    [PDFFieldName("Relationship3")]
    public string relationship3 { get; set; }
    [PDFFieldName("Relationship4")]
    public string relationship4 { get; set; }
    [PDFFieldName("Relationship5")]
    public string relationship5 { get; set; }
    [PDFFieldName("Relationship6")]
    public string relationship6 { get; set; }
    [PDFFieldName("SSN")]
    public string ssn { get; set; }
    public string signature { get; set; }

}

Example controller method:

public IActionResult ShareMedicalInfo(ShareMedicalInfoResponse response)
    {
        var basepath = _appEnvironment.ApplicationBasePath;
        var filePath = @"\App_Data\forms\Information.pdf";

        return new FileContentResult(ITextPDFGenerator.GeneratePDF(basepath, filePath, response), "application/pdf")
        {
            FileDownloadName = "Information.pdf"
        };
    }

EDIT

I've been experimenting a bit, and tried to create a more factory-type version of my above code. I'm not sure if it's a better solution. But it feels somewhat "cleaner".

Interfaces

public interface IPDFBuilder : IDisposable
{
    byte[] GeneratePDF();
    byte[] GenerateAndSignPDF(string certificateLocation);
}

public interface IPDFForm
{
    string Name { get; }
    string FileName { get; }
}

Factory methods

public static class PDFBuilderFactory
{
    public static IPDFBuilder GetPDFBuilder(string filePath, IPDFForm pdfForm)
    {
        if (pdfForm.Name == "Random Class") return new ExamplePDFBuilder (filePath,pdfForm);

        return new GenericPDFBuilder(filePath, pdfForm);
    }
}

public abstract class PDFBuilder : IPDFBuilder
{
    private byte[] PdfTemplate;
    private PdfReader PdfReader;
    private MemoryStream NewPdf;
    private PdfStamper PdfStamper;
    private IPDFForm Request;

    private char[] PASSWORD = "FAKEPASSWORD".ToCharArray();
    protected PDFBuilder() { }

    protected PDFBuilder(byte[] pdfTemplate, IPDFForm request)
    {
        PdfTemplate = pdfTemplate;
        Request = request;
    }

    protected PDFBuilder(string pdfTemplate, IPDFForm request) : this(File.ReadAllBytes(pdfTemplate), request) { }

    public virtual byte[] GeneratePDF()
    {
        PdfReader = new PdfReader(PdfTemplate);
        NewPdf = new MemoryStream();
        PdfStamper = new PdfStamper(PdfReader, NewPdf);

        try
        {
            PopulateFormFields();
            NewPdf.Flush();

            PdfStamper.Close();

            return NewPdf.ToArray();
        }
        catch (Exception ex)
        {
            Console.WriteLine(ex);
        }
        finally
        {

            PdfReader.Close();
            NewPdf.Close();
        }

        return PdfTemplate;
    }


    public virtual byte[] GenerateAndSignPDF(string certificateLocation)
    {
        var pdf = GeneratePDF();

        var finalPdf = pdf;
        var signature = Request.GetType().GetProperty("signature");

        if (signature != null)
        {
            var signatureString = Request.GetType()?.GetProperty(nameof(signature))?.GetValue(Request, null)?.ToString() ?? "";

            finalPdf = SignPDF(certificateLocation, finalPdf, signatureString, "Signature");
        }

        return finalPdf;
    }

    public virtual void PopulateFormFields()
    {
        var pdfFormFields = PdfStamper.AcroFields;
        var properties = Request.GetType().GetProperties();

        pdfFormFields.SetField("Date", DateTime.Now.ToString("MM/dd/yyyy"));

        foreach (PropertyInfo property in properties)
        {
            var attr = (PDFFieldName)Attribute.GetCustomAttribute(property, typeof(PDFFieldName));

            if (attr != null)
            {
                var prop = property.GetValue(Request)?.ToString() ?? "";

                pdfFormFields.SetField(attr.Name, ShapeValues(prop));
            }
        }
    }

    public virtual string ShapeValues(string value)
    {
        if (value.ToLower() == "true") return "On";

        return value;
    }

    public virtual byte[] Sign(byte[] src, ICollection<X509Certificate> chain, ICipherParameters pk, string digestAlgorithm, CryptoStandard subfilter, string fieldname, string reason, string location, string signature)
    {
        var reader = new PdfReader(src);

        PdfReader.unethicalreading = true;

        var fields = reader.AcroFields;
        var newPdf = new MemoryStream();
        var stamper = PdfStamper.CreateSignature(reader, newPdf, '\0');
        var appearance = SetSignatureAppearance(stamper, fieldname, reason, location, signature);

        IExternalSignature pks = new PrivateKeySignature(pk, digestAlgorithm);
        MakeSignature.SignDetached(appearance, pks, chain, null, null, null, 0, subfilter);
        newPdf.Flush();

        return newPdf.ToArray();
    }

    public virtual PdfSignatureAppearance SetSignatureAppearance(PdfStamper stamper, string fieldname, string reason, string location, string signature)
    {
        var appearance = stamper.SignatureAppearance;

        appearance.Reason = reason;
        appearance.Location = location;
        appearance.Layer2Text = signature;
        appearance.Layer2Font = new Font(Font.FontFamily.HELVETICA);
        appearance.Acro6Layers = true;
        appearance.SetVisibleSignature(fieldname);

        return appearance;
    }

    public virtual byte[] SignPDF(string certificatePath, byte[] pdfSource, string signature, string signingReason = "Signature")
    {
        try
        {
            var cert = new FileStream(certificatePath, FileMode.Open);
            var store = new Pkcs12Store(cert, PASSWORD);
            var alias = "";
            ICollection<X509Certificate> chain = new List<X509Certificate>();

            foreach (string al in store.Aliases)
            {
                if (store.IsKeyEntry(al) && store.GetKey(al).Key.IsPrivate)
                {
                    alias = al;
                    break;
                }
            }

            foreach (X509CertificateEntry c in store.GetCertificateChain(alias))
            {
                chain.Add(c.Certificate);
            }

            var pk = store.GetKey(alias);
            var parameters = pk.Key as RsaPrivateCrtKeyParameters;

            cert.Close();

            return Sign(pdfSource, chain, parameters, DigestAlgorithms.SHA256, CryptoStandard.CMS, "Signature", signingReason, "Signature", signature);
        }
        catch (Exception ex)
        {
            Console.WriteLine(ex);
        }

        return pdfSource;

    }

    #region IDisposable Support
    private bool disposedValue;

    protected virtual void Dispose(bool disposing)
    {
        if (!disposedValue)
        {
            if (disposing)
            {
                PdfStamper.Dispose();
                PdfReader.Dispose();
                NewPdf.Dispose();
            }


            disposedValue = true;
        }
    }

    ~PDFBuilder()
    {
        Dispose(false);
    }

    // This code added to correctly implement the disposable pattern.
    public void Dispose()
    {
        Dispose(true);
        GC.SuppressFinalize(this);
    }
    #endregion
}

public class ExamplePDFBuilder : PDFBuilder
{
    public ExamplePDFBuilder (byte[] pdfTemplate, IPDFForm request) : base(pdfTemplate, request) { }
    public ExamplePDFBuilder (string pdfTemplate, IPDFForm request) : base(pdfTemplate, request) { }
}

public class GenericPDFBuilder : PDFBuilder
{
    public GenericPDFBuilder(byte[] pdfTemplate, IPDFForm request) : base(pdfTemplate, request) { }
    public GenericPDFBuilder(string pdfTemplate, IPDFForm request) : base(pdfTemplate, request) { }
}

The controller has changed to be

private IActionResult ProcessPDF(IPDFForm request, string path)
{
    using (var builder = PDFBuilderFactory.GetPDFBuilder(path, request))
    {
        var pdf = builder.GenerateAndSignPDF("Location");

        return new FileContentResult(pdf, "application/pdf")
        {
            FileDownloadName = ((IPDFForm)request).FileName
        };
    }
}
public IActionResult ExampleRequest(ExampleRequest request)
{
    var basepath = "PATH";

    return ProcessPDF(request, basepath);
}

I apologize for the overall length, but I was having trouble breaking it into more manageable chunks. The design pattern illustrated above will allow for custom assignment methods and custom creation of the PDF, but it still allows for somewhat generic creation if that's what is required.

\$\endgroup\$
3
\$\begingroup\$

Just a few notes about the second part in your EDIT.


public interface IPDFBuilder : IDisposable
{
  byte[] GeneratePDF();
  byte[] GenerateAndSignPDF(string certificateLocation);
}

The builder should not have the extra GenerateAndSignPDF. Signing it should be part of another module.

Besides the builder is not a real builder. We use the builder pattern as a helper for creating more complex types by usuly providing an easier to use fluent API. This one here should be a PdfFactory. I also don't think it needs to force the implementer to implement the IDisposable interface. Maybe someone can implement it in a way that does not requrie it to be disposed.

As matter of fact you could already implement it without the IDisposable interface because you can use all the fields locally inside the CreatePdf method.

I suggest a simpler API for it:

public interface IPdfFactory
{
    byte[] CreatePdf(string pdfTemplate, IPdfForm request);
}

The other module could be:

public interface IPdfSigner
{
    void SignPdf(byte[] pdf, string certificationFileName);
}

PDFBuilder
ExamplePDFBuilder
GenericPDFBuilder

I'm wondering why you need three of them. You've already implemented everything you need in the PDFBuilder. The other two does not provide any additional functionality.


PopulateFormFields

This method should not be public. Its usage is not clear especially that it does not take any parameters.


public virtual string ShapeValues(string value)
{
  if (value.ToLower() == "true") return "On";

  return value;
}

This might be the most confusing API of all. I give it TRUE and get back On as an answer.

If you need to map values to other values then create a mapper or use a dictionary or give it a clear name like MapValueOrDefault.

But in case you want to keep this then make it case-insensitive.

if (value.Equals("true", StringComparison.OrdinalIgnoreCase))
{
    return "On";
}
\$\endgroup\$
  • \$\begingroup\$ The main reason I've built the other Builders was to enable me to create custom form field mappings. Sometimes more complex logic will be necessary. That's why I made the PopulateFormFields virtual. It allows it to be overridden. \$\endgroup\$ – JD Davis Dec 21 '16 at 21:29
  • \$\begingroup\$ I've updated my original posting with a slight refactoring of the overall code. I believe it's a bit more separated now, and I tried to give explanation regarding my decisions to mark certain methods virtual and such. \$\endgroup\$ – JD Davis Dec 22 '16 at 0:21
3
\$\begingroup\$

You can consider not to call GC.SuppressFinalize by trying to use the "using" statement instead.

using (MemoryStream ms = new MemoryStream()) {
using (PdfStamper stamper = new PdfStamper(reader, ms, '\0', true)) {
// do stuff      
}    
return ms.ToArray();
}

You can consider to use "var" only for anonymous expressions. This way it improves readability. Example it is good to use "var" for some LINQ operations or anonymous expressions. It is not appropriate to use to define string like below

var test = "teststring";
\$\endgroup\$
  • \$\begingroup\$ Could you please expand this answer further so that we understand your reasoning for the considerations that you are suggesting? Thank you. \$\endgroup\$ – Malachi Dec 21 '16 at 19:33
  • \$\begingroup\$ The reasoning on avoiding the using statement was due to me receiving FileStream closed exceptions. I tried every which way and it constantly happened by the time it posted to my MVC controllers. \$\endgroup\$ – JD Davis Dec 21 '16 at 23:37
  • \$\begingroup\$ As for var, I've mainly gotten into the habit due to Roslyn and Resharper, both of which automatically changed them for me. I had found it aided in my own personal readability. \$\endgroup\$ – JD Davis Dec 21 '16 at 23:40

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.