Two versions of PDF-Selector

Question

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:

source path
destination path
starting page number
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.

1^st 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;
    }

}
}

2^nd 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;
    }

}
}

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. — Physics131
– Physics131, Commented May 14, 2018 at 21:24

score 6 · Accepted Answer · 2018-05-15 07:26:05Z

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:

What if the user wants to copy page 6, 9 and pages 11-25?

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"? — Physics131
– Physics131, Commented May 15, 2018 at 12:48
@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. — user73941
– user73941, Commented May 15, 2018 at 13:16

Stack Exchange Network

Two versions of PDF-Selector

1^st Version:

2^nd Version:

1 Answer 1

You must log in to answer this question.

Hot Network Questions

Two versions of PDF-Selector

1st Version:

2nd Version:

1 Answer 1

You must log in to answer this question.

Related

Hot Network Questions

1^st Version:

2^nd Version: