7
\$\begingroup\$

I made a program that merges all the PDFs in a certain map. The program is made so it merges only one sided PDFs. I would like your opinion on the code and how can it be improved.

while True:
    print()
    try:
        map_name = input("Name of the map where PDFs are saved: ")    # map where your documents are 
        initial_path = r"C:\Users\...\Scanned Documents"              # the initial path where all your documents get saved
        path = os.path.join(initial_path, map_name)
        os.chdir(path)
        break

    except (OSError, FileNotFoundError):
        print("There is no map with that name!")
        continue

PDF_name_input = input("Name of your merged PDF: ")
PDF_name = "{0}.pdf".format(PDF_name_input)


PDF_arr = []
for file in os.listdir("."):

    if file.endswith("pdf"):
        PDF_arr.append(file)

merger = PyPDF2.PdfFileMerger()

for element in PDF_sez:
    merger.append(open(element, "rb"))

with open(PDF_name, "wb") as fout:
    merger.write(fout)
\$\endgroup\$
2
  • 1
    \$\begingroup\$ Why do you keep saying "map"? This is called a "directory". Is there some use case context I'm missing? \$\endgroup\$ Aug 9, 2021 at 23:23
  • \$\begingroup\$ map means folder in my native language, thats why. I didn't remember the english word. \$\endgroup\$
    – Murg
    Aug 11, 2021 at 18:47

2 Answers 2

2
\$\begingroup\$

Let's start with some structural feedback. Since this appears to be a standalone script, it would be good to have a shebang added to it. It's also seen as good practice to use if __name__ == "__main__" even if you aren't developing a package at this point in time (you may want to turn it into one). You would also benefit from having the script accept arguments, instead of reading input during the run.

Your exception handling could definitely use some improvements: right now you have 3 different operations that all can go wrong within the same try-block, and you don't do anything useful when you capture neither OSError nor FileNotFoundError - you may as well just not capture these at all in this case.

I don't see why you have a while True block at all. You're just trying a procedure and then immediately breaking. There's no change from one loop to the next.

You've also left out parts of your script here - you're using functions from PyPDF2 but you've never imported that module. And you're iterating over something called PDF_sez that never has been defined. Looking more closely at PDF_sez, I also have no idea what you're doing at the end of the file: you're appending files (not file contents!) to merger which is a PdfFileMerger, and then you write the file PDF_name (again not the contents) to this sequence (or at least I presume it's a sequence since you apparently could append to it...).

Since you're dealing with paths, it would be a good idea to make use of pathlib - that way you also don't have to set up raw strings (e.g. r"C:\Users...\Scanned Documents").

You also have a few hardcoded paths in your code that probably would be better kept separate.

You're also opening a few files which you never close - it's better to use a contextmanager as you already are on the last two lines.

All in all, although the code still is broken, I'd suggest changing it to something like:

#!/usr/bin/env python3
"""Short script to merge PDFs in a single directory."""

from argparse import ArgumentParser
from pathlib import Path

SCANNED_DOCS_DIR = r"C:\Users\...\Scanned Documents"


def merge_pdfs_in(fname: str, directory: str) -> None:
  save_file = Path(fname) + ".pdf"
  directory = Path(directory)
  pdfs = [f for f in directory.iterdir() if f.suffix == ".pdf"]
  merger = PyPDF2.PdfFileMerger()

  for fname in pdf_sez:  # don't know what `pdf_sez` is, should maybe be `pdfs`
    with open(fname, "rb") as f:
      merger.append(f.read())

  with open(save_file + ".pdf", "wb") as f:
    merger.write(f)  # hey, don't blame me; should maybe be f.write(merger)


def main():
  parser = argparse.ArgumentParser()
  parser.add_argument(
    "filename",
    dest="fname",
    help="Filename for merged PDFs",
    type=Path,
  )
  parser.add_argument(
    "-d",
    "--directory",
    help="Directory in which to merge all PDFs",
    default=SCANNED_DOCS_DIR,
    type=Path,
  )
  args = parser.parse_args()
  merge_pdfs_in(**vars(args))


if __name__ == "__main__":
  main()
\$\endgroup\$
1
\$\begingroup\$

This is a short script so it's probably ok to not have everything broken up into functions. In general my advice is more design choices for user input rather than anything very wrong with the python scripting style.

  • As noted by Zachary Vance, what you call map should be named directory.
  • A design choice may be to use command line arguments as input instead of repeatedly asking for user input. This will be easier to incorporate into other scripts.
  • Another design choice is to use initial_path as the current working directory of the program instead of hardcoded. This gets rid of needing to change directory.
  • file.endswith("pdf") may match any kind of file ending in "pdf", not necessarily ".pdf", such as a file with no extension named "pdf". Probably not an issue.
  • PDF_sez appears to be a typo for PDF_arr.
  • Use with open or explicitly close file objects to prevent many file objects from remaining open. You already do this with writing the final PDF.
\$\endgroup\$

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.