3
\$\begingroup\$

I have just written my first program in C#. Please explain:

  • Have I used any bad practices?
  • How could the code be improved?

I would appreciate if provide the changed code so and some comments so that I could see what was wrong and do some further reading to correct my mistakes. The program works fine.

using System;
using System.IO;
using System.Text.RegularExpressions;
using iTextSharp.text.pdf;

namespace pdfStamperMemory
{
    class Program
    {
        static void Main(string[] args)
        {
            // searching for JS based on: http://stackoverflow.com/a/41386971/2657875
            // MemoryStream based on: http://stackoverflow.com/a/23738927/2657875

            byte[] bytes;
            string script;
            string input = Path.GetFullPath(args[0]);
            string output = Path.Combine(Path.GetDirectoryName(input), Path.GetFileNameWithoutExtension(input) + "-itext.pdf");

            using (var ms = new MemoryStream())
            {
                using (var reader = new PdfReader(input))
                {
                    using (var stamper = new PdfStamper(reader, ms))
                    {
                        // get all page labels
                        string[] labels = PdfPageLabels.GetPageLabels(reader);
                        string[] arr = new string[labels.Length];

                        for (int i = 0; i < labels.Length; i++)
                        {
                            arr[i] += labels[i];
                            if ((arr[0].Equals("Cover")) && i >= 1)
                            {
                                arr[i] = arr[i].Remove(0, 5);
                            }
                        }

                        for (int i = 1; i <= reader.NumberOfPages; i++)
                        {
                            // Get a page a PDF page
                            PdfDictionary page = reader.GetPageN(i);

                            // Get all the annotations of page i
                            PdfArray annotsArray = page.GetAsArray(PdfName.ANNOTS);

                            // If page does not have annotations
                            if (annotsArray == null)
                            {
                                continue;
                            }

                            // For each annotation
                            for (int j = 0; j < annotsArray.Size; ++j)
                            {
                                // For current annotation
                                PdfDictionary curAnnot = annotsArray.GetAsDict(j);

                                // check if has JS
                                PdfDictionary annotAction = curAnnot.GetAsDict(PdfName.A);
                                if (annotAction == null)
                                {
                                    Console.Write("Page {0} annotation {1}: no action\n", i, j);
                                }

                                // test if it is a JavaScript action
                                else if (PdfName.JAVASCRIPT.Equals(annotAction.Get(PdfName.S)))
                                {
                                    PdfObject scriptObject = annotAction.GetDirectObject(PdfName.JS);
                                    if (scriptObject == null)
                                    {
                                        continue;
                                    }
                                    if (scriptObject.IsString())
                                        script = ((PdfString)scriptObject).ToUnicodeString();
                                    else if (scriptObject.IsStream())
                                    {
                                        using (MemoryStream stream = new MemoryStream())
                                        {
                                            ((PdfStream)scriptObject).WriteContent(stream);
                                            script = stream.ToString();
                                        }
                                    }
                                    else
                                    {
                                        Console.WriteLine("Page {0} annotation {1}: malformed JS entry\n", i, j);
                                        continue;
                                    }
                                    if (script.Contains("if (this.hostContainer"))
                                    {
                                        Regex regex = new Regex(@"pp_(.*)'");
                                        Match text2search = regex.Match(script);
                                        if (text2search.Success)
                                        {
                                            //this is a page *label*, but it needs a *number*
                                            //to use PdfAction.GotoLocalPage
                                            string pageLabel = text2search.Groups[1].Value;

                                            // get index of a page label                                            
                                            int labelIndex = Array.IndexOf(arr, pageLabel);
                                            // replace JS with GotoLocalPage
                                            if (labelIndex != -1)
                                            {
                                                // ++ because Array.IndexOf is zer0-based
                                                labelIndex++;
                                                PdfAction action = PdfAction.GotoLocalPage(labelIndex, new PdfDestination(PdfDestination.XYZ, 0, reader.GetPageSize(labelIndex).Height, 1.25f), stamper.Writer);
                                                curAnnot.Put(PdfName.A, action);
                                            }

                                        }
                                    }
                                }
                            }
                        }
                        stamper.SetFullCompression();
                    }
                }
                // grab the bytes before closing things out
                bytes = ms.ToArray();
            }
            try
            {
                File.WriteAllBytes(output, bytes);
                Console.WriteLine("Done!");
            }
            catch
            {
                Console.WriteLine("Cannot save the file!");
            }
            finally
            {
                Console.ReadKey();
            }
        }
    }
} 
\$\endgroup\$

2 Answers 2

3
\$\begingroup\$

Good things

  • You are using the using statement to properly dispose objects which are implementing IDisposable.
  • You are using braces {} althought they might be optional

Improvable things

  • Comments...if used they should describe why something is done in the way it is done. E.g showing edge cases, explain why a workaround of some kind is used.

    Let the code itself tell the reader what is done by using meaningful and descriptive names for variables, methods and classes.

  • Naming things shouldn't involve abbreviations. For example look at these lines

    // For current annotation  
    PdfDictionary curAnnot = annotsArray.GetAsDict(j);  
    

    Why don't you name it currentAnnotation ? This would make the comment superflous.

  • Declarations of variables should be done as near as possible to their usage.Having e.g byte[] bytes; at the top of the method but using it only at the bottom of the method is not optimal.

  • If the right hand side of an assignement make the type of an object clear you should consider to use the var type.


Now let us dig a little bit into the code.

        using (var ms = new MemoryStream())
        {
            using (var reader = new PdfReader(input))
            {
                using (var stamper = new PdfStamper(reader, ms))
                {  

Using using is very good, but in this case it could/should be improved by stacking the usings like so

        using (var ms = new MemoryStream())
        using (var reader = new PdfReader(input))
        using (var stamper = new PdfStamper(reader, ms))
        {  

this will save you two levels of indentation which prevents you from the need to scroll horizontally to see all of the code.


You have one big method where you stuffed in everything. You should check what parts of the code could be easily placed into a method.

Take for example this

        // get all page labels
        string[] labels = PdfPageLabels.GetPageLabels(reader);
        string[] arr = new string[labels.Length];

        for (int i = 0; i < labels.Length; i++)
        {
            arr[i] += labels[i];
            if ((arr[0].Equals("Cover")) && i >= 1)
            {
                arr[i] = arr[i].Remove(0, 5);
            }
        }  

by using a method like this

private static string[] GetPageLabels(PdfReader reader)
{
    string[] labels = PdfPageLabels.GetPageLabels(reader);
    if (!labels[0].Equals("Cover"))
    {
        return labels;
    }
    for (int i = 1; i < labels.Length; i++)
    {
        labels[i] = labels[i].Remove(0, 5);
    }  
    return labels;
}  

your main method will become shorter.

In general you should use more and shorter methods. This will makes it easier to read and understand the code and if you are hunting for a bug it will be easier to find.


        PdfDictionary annotAction = curAnnot.GetAsDict(PdfName.A);
        if (annotAction == null)
        {
            Console.Write("Page {0} annotation {1}: no action\n", i, j);
        }

        // test if it is a JavaScript action
        else if (PdfName.JAVASCRIPT.Equals(annotAction.Get(PdfName.S)))

This is a big no go. Don't place comments between an if and an else if. You or Sam the maintainer won't be able to see at first glance that they (if..else if) belong together.


If usingt the same regex like Regex regex = new Regex(@"pp_(.*)'"); in a loop you should consider to create the regex outside of the loop with this overloaded constructor using RegexOptions.Compiled

Specifies that the regular expression is compiled to an assembly. This yields faster execution but increases startup time.


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

More improvable things

for (int i = 1; i <= reader.NumberOfPages; i++)

Try to use a constant in cases like this because it's not obvious why you start iterating from 1.


Regex regex = new Regex(@"pp_(.*)'");

You should either instantiate it once like @Heslacher suggested or you can use static Regex.Match method. It'll cache the expression so it'll be quite fast too.

The static Match(String, String) method is equivalent to constructing a Regex object with the specified regular expression pattern and calling the instance Match(String) method. In this case, the regular expression engine caches the regular expression pattern.


You can make the regex easier to read and use by naming the group so this

string pageLabel = text2search.Groups[1].Value;

would become

string pageLabel = text2search.Groups["searchString"].Value;

if you define the expression like

var text2SearchMatch = Regex.Match(script, @"pp_(?<searchString>.*)'");

// replace JS with GotoLocalPage
if (labelIndex != -1)

Instead of writing comments (they tend to rot) it's often better to use a helper variable:

var someLabelExsits = labelIndex != -1;
if (!someLabelExsits)
\$\endgroup\$
0

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge you have read our privacy policy.

Not the answer you're looking for? Browse other questions tagged or ask your own question.