1
\$\begingroup\$

This is related to my (unanswered) question here, but the code has changed considerably since I asked that question. I have a method that takes the filename of a PDF file and a Script object, and applies the Script to the PDF. If you are not familiar, PDFs support JavaScript to perform various tasks such as changing the content of documents, and scripts can be executed on various events such as on printing or saving.

A Script has a ScriptText, a ScriptField, and a ScriptTiming. The text is the JavaScript itself, the field is an object with a string Title and four integers representing the coordinates of the field on the document, and the timing is an enum value used to determine when the script should be executed.

The method to add the script to the PDF is as follows:

private void AddScriptToPdf(string filename, Script script)
{
    try
    {
        using (var pdfReader = new iTextSharp.text.pdf.PdfReader(filename))
        {
            using (var pdfStamper = new iTextSharp.text.pdf.PdfStamper(
                    pdfReader,
                    new System.IO.FileStream(
                        GetOutputPath(filename),
                        System.IO.FileMode.Create)))
            {
                var parentField
                    = iTextSharp.text.pdf.PdfFormField.CreateTextField(
                        pdfStamper.Writer,
                        false,
                        false,
                        0);
                parentField.FieldName = script.ScriptField.Title;
                for (var pageNumber = PdfFirstPageNumber;
                    pageNumber <= pdfReader.NumberOfPages;
                    pageNumber++)
                {
                    var pdfContentByte = pdfStamper.GetOverContent(
                        pageNumber);
                    var textField = new iTextSharp.text.pdf.TextField(
                            pdfStamper.Writer,
                            new iTextSharp.text.Rectangle(
                                script.ScriptField.TopLeftX,
                                script.ScriptField.TopLeftY,
                                script.ScriptField.BottomRightX,
                                script.ScriptField.BottomRightY),
                            null);
                    var childField = textField.GetTextField();
                    parentField.AddKid(childField);
                    childField.PlaceInPage = pageNumber;
                }
                pdfStamper.AddAnnotation(parentField, 1);
                var pdfAction
                    = iTextSharp.text.pdf.PdfAction.JavaScript(
                        script.ScriptText,
                        pdfStamper.Writer);
                iTextSharp.text.pdf.PdfName actionType = null;
                switch (script.ScriptTiming)
                {
                    case ScriptTiming.DidPrint:
                        actionType
                            = iTextSharp.text.pdf.PdfWriter.DID_PRINT;
                        break;

                    case ScriptTiming.DidSave:
                        actionType
                            = iTextSharp.text.pdf.PdfWriter.DID_SAVE;
                        break;

                    case ScriptTiming.WillPrint:
                        actionType
                            = iTextSharp.text.pdf.PdfWriter.WILL_PRINT;
                        break;

                    case ScriptTiming.WillSave:
                        actionType
                            = iTextSharp.text.pdf.PdfWriter.WILL_SAVE;
                        break;
                }
                pdfStamper.Writer.SetAdditionalAction(
                    actionType,
                    pdfAction);
            }
        }
    }
    catch (iTextSharp.text.exceptions.InvalidPdfException e)
    {
        ShowMessage(e.Message + filename);
    }
}

The method is long, but requires very little logic. It's pretty much step-by-step instructions. I know I could split it into several smaller methods, but considering they only ever run in sequence, I don't see the point of that. Is there any way I could actually make it better (or shorter)?

\$\endgroup\$
3
\$\begingroup\$
  • There is an excessive amount of new line after parameter delimiter(,) and assignment operator(=). At the same time, there is no new line between logical blocks of the method. Together, it makes the code difficult to read when glancing over.
  • When using two directly nested using, you can write it as the following to reduce the level of indentation :

    using (var pdfReader = new PdfReader(filename))
    using (var pdfStamper = new PdfStamper(pdfReader, new FileStream(GetOutputPath(filename), FileMode.Create)))
    {
        // ...
    }
    
  • The switch can be replaced by a dictionary to remove the boilerplate breaks :

    var actionMapping = new Dictionary<ScriptTiming, PdfName>
    {
        [ScriptTiming.DidPrint] = PdfWriter.DID_PRINT,
        [ScriptTiming.DidSave] = PdfWriter.DID_SAVE,
        [ScriptTiming.WillPrint] = PdfWriter.WILL_PRINT,
        [ScriptTiming.WillSave] = PdfWriter.WILL_SAVE,
    };
    
  • You have some very-very-very-lonnnng name due to lack of using namespace :

    new iTextSharp.text.Rectangle(...)
    iTextSharp.text.pdf.PdfFormField.CreateTextField(...)
    
    // which can be replaced by this
    new Rectangle(...)
    PdfFormField.CreateTextField(...)
    
    // if you add this line on the top of the file
    using iTextSharp.text;
    using iTextSharp.text.pdf;
    
    // if the compiler complains that Rectangle is ambiguous, use this instead
    //using Rectangle = iTextSharp.text.Rectangle;
    

Code :

using iTextSharp.text;
using iTextSharp.text.pdf;

private void AddScriptToPdf(string filename, Script script)
{
    try
    {
        using (var pdfReader = new PdfReader(filename))
        using (var pdfStamper = new PdfStamper(pdfReader, File.Create(GetOutputPath(filename))))
        {
            var parentField = PdfFormField.CreateTextField(pdfStamper.Writer, false, false, 0);
            parentField.FieldName = script.ScriptField.Title;

            /* Some space here*/
            for (var pageNumber = PdfFirstPageNumber; pageNumber <= pdfReader.NumberOfPages; pageNumber++)
            {
                //var pdfContentByte = pdfStamper.GetOverContent(pageNumber); // <-- as @Heslacher pointed out, this is never unsed
                var textField = new TextField(
                    pdfStamper.Writer,
                    new Rectangle(
                        script.ScriptField.TopLeftX,
                        script.ScriptField.TopLeftY,
                        script.ScriptField.BottomRightX,
                        script.ScriptField.BottomRightY),
                    null);

                var childField = textField.GetTextField();
                childField.PlaceInPage = pageNumber;
                parentField.AddKid(childField);
            }

            /* Optional: Some space here */
            pdfStamper.AddAnnotation(parentField, 1);

            /* Some more space here */
            var pdfAction = PdfAction.JavaScript(script.ScriptText, pdfStamper.Writer);
            var actionMapping = new Dictionary<ScriptTiming, PdfName>
            {
                [ScriptTiming.DidPrint] = PdfWriter.DID_PRINT,
                [ScriptTiming.DidSave] = PdfWriter.DID_SAVE,
                [ScriptTiming.WillPrint] = PdfWriter.WILL_PRINT,
                [ScriptTiming.WillSave] = PdfWriter.WILL_SAVE,
            };
            // if null needs to be passed for any other cases, use the commented lines instead
            pdfStamper.Writer.SetAdditionalAction(actionType, actionMapping[script.ScriptTiming]);
//            pdfStamper.Writer.SetAdditionalAction(actionType, actionMapping
//                .Where(x => x.Key == script.ScriptTiming)
//                .Select(x => x.Value)
//                .FirstOrDefault());
        }
    }
    catch (InvalidPdfException e)
    {
        ShowMessage(e.Message + filename);
    }
}
\$\endgroup\$
  • \$\begingroup\$ My reason for breaking the lines is to keep the lines of code at 80 characters or less. It may look shorter in my post, but that is because the namespace nesting adds an additional indentation level, which I remove to post the code here. Additionally, I was avoiding using namespace to prevent complications. Several of the namespaces I am using share class names, and so I already had to fully specify a few. Is this also not true for C#: stackoverflow.com/a/1452738/5004298 \$\endgroup\$ – Michael Brandon Morris Jun 2 '16 at 10:13
  • 1
    \$\begingroup\$ @MichaelBrandonMorris It is also true for C#, however, you can always alias namespace or class separately to avoid complication like ambiguous name : using text = iTextSharp.text; and using Rectangle = iTextSharp.text.Rectangle;. Keep it under 80 characters is a good one, but consider placing the = on the same line with the variable. So, your indentation isn't offset by 2characters("= "). \$\endgroup\$ – Xiaoy312 Jun 2 '16 at 15:23
2
\$\begingroup\$

It seems that

var pdfContentByte = pdfStamper.GetOverContent(
                    pageNumber);  

is never used so you should get rid of it.


Although this code is always executed in sequence it is better to extract separate stuff to methods. This will help if you need to find a bug or if you are trying to refactor it.

In its current state that method is doing to much. It is constructing and adding the textfields (annotations) and is asigning the javascript functionality.


The readability of that method should be improved by adding vertical spaces(new lines). Using vertical space will help to group related code and makes it more structured.

\$\endgroup\$

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.