Changing PDF annotation with JavaScript code to jump to page into regular to Go To Page action

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 = 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

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 = 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.

StackExchange Code Review Q#154117, answer score: 3