import re
from collections import Counter

def reader(filename):

    with open(filename) as f:

        file = f.read()

        regex = r'\d{1,3}\.\d{1,3}\.\d{1,3}\.\d{1,3}'

        final = re.findall(regex, file)

        return final

def count(final):


if __name__ == '__main__':

    file_name = input("Enter file name: ")

  • 5
    \$\begingroup\$ Please add some explanatory context. \$\endgroup\$
    – Reinderien
    Commented Mar 4, 2022 at 12:42
  • 6
    \$\begingroup\$ Please read How do I ask a good question?. The code review should be from a working project you have written. The title should explain what the code does. \$\endgroup\$
    – pacmaninbw
    Commented Mar 4, 2022 at 12:49

1 Answer 1


If you are not planning to reuse these functions elsewhere, it does not make a whole lot of sense to split the logic like so. It does not make a lot of sense to use this python script as a CLI tool, either - grep -o | wc -l will do it for you, but I digress.

Feeding the filename after you run the script results in some poor usability - I would rather use argparse or, if you plan to do it interactively, not exit after one run. I would instead go with something like

import re
import argparse

if __name__ == '__main__':
    parser = argparse.ArgumentParser()
    args = parser.parse_args()
    pattern = r'\d{1,3}\.\d{1,3}\.\d{1,3}\.\d{1,3}'
    with open(args.filename) as f:
        print(len(re.findall(pattern, f.read())))
  • 1
    \$\begingroup\$ I disagree with the function-reuse statement. By its nature, Python allows for reasonable unit testing at the script level when there is a sane function structure. By comparison, your proposal is a huge pain to test. \$\endgroup\$
    – Reinderien
    Commented Mar 4, 2022 at 13:48
  • \$\begingroup\$ Said another way: the whole point of a main guard (which you have preserved) is to allow for outside imports of the functions in this file. \$\endgroup\$
    – Reinderien
    Commented Mar 4, 2022 at 13:50
  • \$\begingroup\$ @Reinderien Oh, yes, sorry. I agree; my point was rather that in how OP has presented their code, I (personally) see little merit in having reader() and count() functions defined like so - if anything, it would be probably better to make something like count_ip_addresses(filename) and combine the functionality of two in there. At least this is my sneaking suspicion. Given the use case is extremely non-obvious here, I would stick to the "you won't need it" principle: refactor when it is clear how the code will be reused and do not overcomplicate things at start. \$\endgroup\$
    – Lodinn
    Commented Mar 4, 2022 at 15:07

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