Mfcttrf

I created a simple script that implements log rotation. It checks the log file size (say, out.log) if it exceeds 5MB limit. If it does then the script copies it to out1.log and clears out.log. But it also copies out1.log to out2.log, etc. So log files are kind of 'shifted' and contents of the last one is discarded. In this implementation I keep only 5 files (initial out.log and out[1-4].log).

I am not a Python programmer, although I find this language extremely useful for such kinds of tasks. I would like my code to be as much idiomatic as possible, so what can I change/improve in my script for this?

from os import listdir
from os.path import getsize, exists
from shutil import copyfile
from argparse import ArgumentParser

SIZE_5MB = 5e6
MAX_LOG_FILES_COUNT = 5

class LogRotator(object):
 def __init__(self, prefix, suffix):
 self.prefix = prefix
 self.suffix = suffix

 def __str__(self):
 return "[x].".format(self.suffix, self.prefix)

 def __touch(self, file_name):
 open(file_name, 'w').close()

 def rotate(self):
 files = [".".format(self.prefix, x + 1, self.suffix) for x in range(MAX_LOG_FILES_COUNT)]

 [self.__touch(f) for f in files if not exists(f)]

 current_log = ".".format(self.prefix, self.suffix)
 if (getsize(current_log) < SIZE_5MB):
 return

 files.insert(0, current_log)

 for i in range(len(files) - 1, 0, -1):
 copyfile(files[i-1], files[i])

 self.__touch(files[0])

if __name__ == '__main__':
 parser = ArgumentParser(description="Rotate log files")
 parser.add_argument("-prefix", help="log file prefix")
 parser.add_argument("-suffix", help="log file suffix")

 args = parser.parse_args()

 logRotator = LogRotator(args.prefix, args.suffix)
 logRotator.rotate()

1. Return early, there's no need to touch files if the current file is <5mb.


2. DRY your code, move the ".".format into it's own function.


3. Don't use comprehensions for mutations. Use an explicit for loop for that.


4. 
   

   As you want idiomatic code, you should be aware that some people discourage the use of __ as it performs name mangling.

   
   
   

   If the function is intended to be private, not protected, then it should be ok.

   
   


5. You can use the pairwise recipe to make the reversed pairwise loop. I think it's more readable, however you may not.


6. It's unidiomatic to put brackets on if statements. Unless the statement spans multiple lines.


7. It's idiomatic to put two newlines around top level classes and functions.


8. I'm a bit confused why you want to touch everything.


9. You may want to split all the different aspects of rotate into smaller functions to follow SRP.

Your code's pretty idiomatic otherwise, your naming conventions are good, you've used a main guard and you've got white space where it should be.

from os import listdir
from os.path import getsize, exists
from shutil import copyfile
from argparse import ArgumentParser
import itertools


def pairwise(iterable):
 "s -> (s0,s1), (s1,s2), (s2, s3), ..."
 a, b = itertools.tee(iterable)
 next(b, None)
 return zip(a, b)


SIZE_5MB = 5e6
MAX_LOG_FILES_COUNT = 5


class LogRotator(object):
 def __init__(self, prefix, suffix):
 self.prefix = prefix
 self.suffix = suffix

 def __str__(self):
 return "[x].".format(self.suffix, self.prefix)

 def __touch(self, file_name):
 open(file_name, 'w').close()

 def _gen_file_name(self, name):
 return ".".format(self.prefix, name, self.suffix)

 def rotate(self):
 current_log = self._gen_file_name('')
 if getsize(current_log) < SIZE_5MB:
 return

 files = [
 self._gen_file_name(i)
 for i in range(1, MAX_LOG_FILES_COUNT + 1)
 ]

 for file in files:
 if not exests(file):
 self.__touch(f)

 for older, newer in pairwise(reversed([current_log] + files)):
 copyfile(newer, older)

 self.__touch(current_log)


if __name__ == '__main__':
 parser = ArgumentParser(description="Rotate log files")
 parser.add_argument("-prefix", help="log file prefix")
 parser.add_argument("-suffix", help="log file suffix")

 args = parser.parse_args()

 log_rotator = LogRotator(args.prefix, args.suffix)
 log_rotator.rotate()