6
\$\begingroup\$

I have basically ended up with 2 versions of a PDF-Selector. Both versions do the same thing. I'm not sure which solution is better. Maybe someone could give me a code review.

I use: PDFSharp-Nuget-Package, Visual Studio community 2017, Resharper

My program takes 4 inputs:

  1. source path
  2. destination path
  3. starting page number
  4. ending page number

The original PDF is loaded from the source path and split into a new PDF file to the destination path. Only page numbers between the starting page number and ending page number will be saved.

1st Version:

using System;
using System.Diagnostics;
using PdfSharp.Pdf;
using PdfSharp.Pdf.IO;


namespace PdfSplitter_V1
{
class ProgramV1
{
    static void Main(string[] args)
    {

        Console.WriteLine("this application stores selected pages of a pdf-file");

        PdfDocument inputDocument = new PdfDocument();
        PdfDocument outputDocument = new PdfDocument();

        string inputPath = "";
        string outputPath = "";
        inputPath = GetValues.GetPath("Enter a valid input path", inputPath, outputPath);
        outputPath = GetValues.GetPath("where do you want to save the file? enter a unique filename!", inputPath, outputPath);

        inputDocument = PdfReader.Open(inputPath, PdfDocumentOpenMode.Import);
        int countPages = inputDocument.PageCount;

        int min = GetValues.GetMin("Enter the starting page", countPages);
        int max = GetValues.GetMax("Enter the ending page", countPages, min);

        for (int i = min-1; i < max; i++)
        {
            outputDocument.AddPage(inputDocument.Pages[i]);
        }


        outputDocument.Save(outputPath);
        Process.Start(outputPath);
    }
}

public class GetValues
{
    public static string GetPath(string prompt, string inputPath, string outputPath)
    {
        string path;
        do
        {
            path = GetInputs.GetString(prompt);
        } while (path.Equals(inputPath) || path.Equals(outputPath));
        return path;
    }

    public static int GetMin(string prompt, int countPages)
    {
        int min;
        do
        {
            min = GetInputs.GetInt(prompt);
        } while (min > countPages);

        return min;
    }

    public static int GetMax(string prompt, int countPages, int min)
    {
        int max;
        do
        {
            max = GetInputs.GetInt(prompt);
        } while ((max < min) && (max > countPages));

        return max;
    }
}

public static class GetInputs
{
    public static string GetString(string prompt)
    {
        string input;

        do
        {
            Console.WriteLine(prompt);
            input = Console.ReadLine();
        } while (String.IsNullOrEmpty(input));

        return input;
    }

    public static int GetInt(string prompt)
    {
        string input;
        int output;

        do
        {
            Console.WriteLine(prompt);
            input = Console.ReadLine();
        } while (!int.TryParse(input, out output) || (output <= 0));

        return output;
    }

}
}

2nd Version:

using System;
using System.Diagnostics;
using PdfSharp.Pdf;
using PdfSharp.Pdf.IO;

namespace PdfSplitter
{
class ProgramV2
{
    static void Main(string[] args)
    {
        Console.WriteLine("this application stores selected pages of a pdf-file");

        PdfDocument inputDocument;
        PdfDocument outputDocument = new PdfDocument();

        GetValue newGetValue = new GetValue();

        Tuple<string, string> paths = newGetValue.GetPaths("Enter a valid input path", "Enter a valid output path");
        string inputPath = paths.Item1;
        string outputPath = paths.Item2;

        inputDocument = PdfReader.Open(inputPath, PdfDocumentOpenMode.Import);
        int countPages = inputDocument.PageCount;

        const string promptMin = "Enter a starting page";
        const string promptMax = "Enter a ending page";
        Tuple<int, int>minMax = newGetValue.GetMinMax(promptMin, promptMax, countPages);
        int min = minMax.Item1;
        int max = minMax.Item2;

        for (int i = min - 1; i < max; i++)
        {
            outputDocument.AddPage(inputDocument.Pages[i]);
        }

        outputDocument.Save(outputPath);
        Process.Start(outputPath);

        Console.WriteLine("Console.Readline");
        Console.ReadLine();
    }
}

public class GetValue
{
    private string m_inputPath { get; set; }
    private string m_outputPath { get; set; }

    private int m_min { get; set; }
    private int m_max { get; set; }

    public Tuple<string, string> GetPaths(string promptInputPath, string promptOutputPath)
    {

        while (String.IsNullOrEmpty(m_inputPath))
        {
            m_inputPath = GetInputs.GetString(promptInputPath);
        }

        while (String.IsNullOrEmpty(m_outputPath) || m_outputPath.Equals(m_inputPath))
        {
            m_outputPath = GetInputs.GetString(promptOutputPath);
        }

        return Tuple.Create(m_inputPath, m_outputPath);
    }

    public Tuple<int, int> GetMinMax(string promptMin, string propmtMax, int countPages)
    {

        do
        {
            m_min = GetInputs.GetInt(promptMin);
        } while (m_min > countPages);

        do
        {
            m_max = GetInputs.GetInt(propmtMax);
        } while (m_max < m_min || m_max > countPages);

        return Tuple.Create(m_min, m_max);
    }

}

public static class GetInputs
{
    public static string GetString(string prompt)
    {
        string input;

        do
        {
            Console.WriteLine(prompt);
            input = Console.ReadLine();
        } while (String.IsNullOrEmpty(input));

        return input;
    }

    public static int GetInt(string prompt)
    {
        string input;
        int output;

        do
        {
            Console.WriteLine(prompt);
            input = Console.ReadLine();
        } while (!int.TryParse(input, out output) || (output <= 0));

        return output;
    }

}
}
\$\endgroup\$
  • \$\begingroup\$ If there is a way to improve to get a review then please tell me that. Like adding comments for example. The reason why i didnt add some, is that i always try to programm self explaing code. Thank you in advance. And thanks to the people who edited my post, to provide a better readability. \$\endgroup\$ – bryan kon May 14 '18 at 21:24
6
\$\begingroup\$

There is a little but important error here:

  public static int GetMax(string prompt, int countPages, int min)
  {
    int max;
    do
    {
      max = GetInputs.GetInt(prompt);
    } while ((max < min) && (max > countPages));

    return max;
  }
}

The condition should be: while ((max < min) || (max > countPages));


In general it is fine to loop the prompt for input until you get a valid answer, but as a user I find it irritating not having an opportunity to cancel the input loop (and the whole process):

  public static int GetInt(string prompt)
  {
    string input;
    int output;

    do
    {
      Console.WriteLine(prompt);
      input = Console.ReadLine();
    } while (!int.TryParse(input, out output) || (output <= 0));

    return output;
  }

You could do something like:

  public static int GetInt(string prompt)
  {
    string input;
    int output;

    do
    {
      Console.WriteLine($"{prompt} (Q to quit):");
      input = Console.ReadLine();
      if (input == "Q") 
      {
         throw new UserAbortedException(prompt);
         // or any other way to exit the loop in a graceful manner
      }
    } while (!int.TryParse(input, out output) || (output <= 0));

    return output;
  }

This is a rather strange way to handle and check path input because it is not obvious what purpose inputPath and outputPath have:

  public static string GetPath(string prompt, string inputPath, string outputPath)
  {
    string path;
    do
    {
      path = GetInputs.GetString(prompt);
    } while (path.Equals(inputPath) || path.Equals(outputPath));
    return path;
  }

A more self explaining version could be something like:

  public static string GetPath(string prompt, bool checkExistance, params string[] exclude)
  {
    string path;
    do
    {
      path = GetInputs.GetString(prompt);
    } while (checkExistance && !File.Exists(path) || exclude.Contains(path));
    return path;
  }

You could split up the main function into meaningful sub routines like:

  try
  {
    Console.WriteLine("This application stores selected pages of a pdf-file");

    var filePaths = GetFilePaths();

    PdfDocument inputDocument = OpenPdfDocument(filePaths.InputPath, PdfDocumentOpenMode.Import);

    var pages = GetPages(inputDocument.PageCount);

    CopyPages(pages, inputDocument, filePaths.OutputPath);

    Process.Start(filePaths.OutputPath);
  }
  catch (Exception ex)
  {
    Console.WriteLine($"Error while copying pages: {ex.Message}");
  }

Where each sub routine has a schema like:

(int StartPage, int EndPage) GetPages(int numPages)
{
  try
  {
    // TODO: prompt for input or do work...
  }
  catch (Exception ex)
  {
    throw new DedicatedException("<Message>", ex);
  }
}

Other things:

  1. What if the user wants to copy page 6, 9 and pages 11-25?
\$\endgroup\$
  • \$\begingroup\$ +1 Great review, and great last question! :) \$\endgroup\$ – Abbas May 15 '18 at 8:55
  • \$\begingroup\$ Wow thank you for the great review. The last question is nice. I think i will implement a "mode select class". But i have still a few questions. It would be nice if i could get a answer on ea. numer of question: 1.) should i go further with version 1 or 2? 2.) I know that tuples could decrease readability and increase complexity. Is it maybe reasonable that i used tuples in version 2, because Min/Max and inputh/outputPath are releated to ea other? 3.) should i add private fields or propertys if i stay with version 1 in the "GetValue-Class"? \$\endgroup\$ – bryan kon May 15 '18 at 12:48
  • \$\begingroup\$ @bryankon: 1) I would go with version 2, but clean up the main function as I suggested above. 2) I think it is OK to use tuples in this rather simple context, but if you code in .NET 4.7++ you should use ValueTuples instead of Tuples. The benefit is, that you then can name the tuple-values as shown above in GetPages(). 3) IMO that would complicate the everything unnecessarily. \$\endgroup\$ – Henrik Hansen May 15 '18 at 13:16

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.