8
\$\begingroup\$

My custom Logger class works just fine. However, the class is not intended for making instances of. Because there is really no reason to make instances of my class, or so I think.

Any constructive criticism? Is this considered good "design"?

I have a feeling that I shouldn't open and close my file for each method call, is that bad? Is the way I open/write/close files in my Logger class bad?

from Time import Time
from datetime import datetime

temp = datetime.now().today()
YEAR, MONTH, DAY = temp.year, temp.month, temp.day

class Logger:
    """If a log file for today already exist, open it in append mode.
    Else, create a new log file for today, and open it in append mode.
    """
    DEBUG = False
    PADDING = 9     # right-padding for type names in the log
    filename = f"logs/log{DAY}-{MONTH}-{YEAR}.txt"
    file = open(filename, "a").close()

    """Clears the log file"""
    @staticmethod
    def clear():
        with open(Logger.filename, "r+") as file:
            file.truncate(0)

    """Log info"""
    @staticmethod
    def log(msg):
        with open(Logger.filename, "a") as file:
            type = "INFO".ljust(Logger.PADDING)
            file.write(f"|{type}|{Time.getHour()}:{Time.getMinute()}| {msg}\n")

    "Used to log whenever a state is updated"
    @staticmethod
    def update(msg):
        with open(Logger.filename, "a") as file:
            type = "UPDATE".ljust(Logger.PADDING)
            file.write(f"|{type}|{Time.getHour()}:{Time.getMinute()}| {msg}\n")

    "Only logs if the static variable {DEBUG} is set to True."
    @staticmethod
    def debug(msg):
        if not Logger.DEBUG: return # Only log if DEBUG is set to True
        with open(Logger.filename, "a") as file:
            type = "DEBUG".ljust(Logger.PADDING)
            file.write(f"|{type}|{Time.getHour()}:{Time.getMinute()}| {msg}\n")

    """Intended for use on objects. They usually have a lot of information;
    therefor, we enclose the information with lines to make the log more readable."""
    @staticmethod
    def object(object):
        with open(Logger.filename, "a") as file:
            file.write(f"-----------------------       object log\n")
            file.write(object)
            file.write(f"\n-----------------------\n")

    @staticmethod
    def exception(msg):
        with open(Logger.filename, "a") as file:
            type = "EXCEPTION".ljust(Logger.PADDING)
            file.write(f"|{type}|{Time.getHour()}:{Time.getMinute()}| {msg}\n")



if __name__ == "__main__":
    Logger.log("This is a test")
    Logger.log("running something")
    Logger.debug("Some debugging details")
    Logger.exception("Critical error!")
    Logger.debug("Some more debugging details")

Output example:

|INFO     |10:35| This is a test
|INFO     |10:35| running something
|DEBUG    |10:35| Some debugging details
|EXCEPTION|10:35| Critical error!
|DEBUG    |10:35| Some more debugging details
\$\endgroup\$
2
  • 2
    \$\begingroup\$ Have you intentionally reinvented the logging module? \$\endgroup\$
    – Peilonrayz
    Commented Jan 16, 2020 at 10:48
  • \$\begingroup\$ @Peilonrayz No, my goal was not to reinvent the logging module. That said, the point of this class is the same as that of the logging module -- to log. I choose to create my own logging class for my project so that I didn't have to deal with the logging module, and so that I could more easily tailor it to do exactly as I wanted. \$\endgroup\$ Commented Jan 16, 2020 at 10:54

3 Answers 3

8
\$\begingroup\$
  • Docstrings should be on the inside of a function or class, not on the outside - before the def or class.
  • Your logger doesn't automatically log to the next day's log. As YEAR, MONTH, DAY and filename are only defined once and never update.
  • It's abnormal to see Logger.filename rather than self.filename or cls.filename. If you need to guarantee it's the class' value rather than the instances than you can do type(self). This makes it harder to rename your class if you ever need to in the future.
  • Your functions should be classmethods as they're interacting with the class. staticmethod is just wrong.
  • Due to a combination of the above two your class can't be instanced or subclassed. This is an artificial limitation and is un-Pythonic.
  • It's normally better to use import x rather than from x import .... The only time I've found this to not be the case, is when you're importing from typing. But every other time I've found code to be more readable. As you know what the function is, without scrolling to the top of the file.
  • There's no benefit to using datetime.now().today() as they return the same object, but with a slightly different time due to the delay in each call.
  • It would be better to define the format once. Currently you're duplicating it across all functions. To do so you would need to pass a datetime / time object and you can just use the format specifier.

    f"|{type}|{datetime:%I:%M}| {msg}\n"
    
  • You don't need to manually call str.ljust you can just change your format to include:

    f"{type:< {Logger.PADDING}}"
    
  • Don't use return on the same line as an if. It's un-Pythonic.

  • PEP 8 - Python's style guide - suggests using only 2 spaces before an inline comments #. You've used 1 and 5.
  • Personally I dislike inline comments as they're hard to read. Putting the comment before the line has normally always been more readable for me.
  • I suggest you rename log to info and allow log handle pretty much everything. For example interaction with the format string and also not logging if it's a debug message.
  • You could just open the file once and leave the file object to be closed when Python exits - as that the only time you'd want to actually close the file.

    If you're sceptical that Python will clean up the file on exit you can just register an exit handler with atexit.

  • Your log levels aren't great. What if I want to ignore non-exceptions?

  • You don't log to std.out or std.err, this is a short coming.
import datetime


class Logger:
    """If a log file for today already exist, open it in append mode.
    Else, create a new log file for today, and open it in append mode.
    """
    DEBUG = False
    PADDING = 9
    FORMAT = "|{type:< {cls.PADDING}}|{datetime:%I:%M}| {msg}\n"
    FILE = open(f"logs/log{datetime.datetime.now():%d-%m-%Y}.txt", "w+")

    @classmethod
    def log(cls, msg, level):
        if not cls.DEBUG and level == "DEBUG":
            return
        cls.FILE.write(cls.FORMAT.format(
            type=level,
            msg=msg,
            datetime=datetime.datetime.now(),
        ))

    @classmethod
    def info(cls, msg):
        """Log info"""
        cls.log(msg, "INFO")

    @classmethod
    def update(cls, msg):
        "Used to log whenever a state is updated"
        cls.log(msg, "UPDATE")

    @classmethod
    def exception(cls, msg):
        cls.log(msg, "EXCEPTION")

    @classmethod
    def debug(cls, msg):
        "Only logs if the static variable {DEBUG} is set to True."
        cls.log(msg, "DEBUG")

    @classmethod
    def clear(cls):
        """Clears the log file"""
        cls.FILE.truncate(0)

    @classmethod
    def object(cls, object):
        """Intended for use on objects. They usually have a lot of information;
        therefor, we enclose the information with lines to make the log more readable."""
        cls.FILE.write(
            f"-----------------------       object log\n"
            + str(object)
            + f"\n-----------------------\n"
        )


if __name__ == "__main__":
    Logger.info("This is a test")
    Logger.info("running something")
    Logger.debug("Some debugging details")
    Logger.exception("Critical error!")
    Logger.debug("Some more debugging details")

Logging

But why not just use logging? You don't have to care if you've handled the file correctly, and you can get pretty much everything you want with ease. The entire of the logging module was created to be highly customizable. It's why it's so ridiculously complex.

As an example of defining UPDATE with a level of 5. And printing everything by default. With some functions removed for brevity.

import datetime
import logging

def _update(self, message, *args, **kwargs):
    if self.isEnabledFor(5):
        self._log(5, message, args, **kwargs)

logging.basicConfig(
    filename=f"logs/log{datetime.datetime.now():%d-%m-%Y}.txt",
    format="|%(levelname)-9s|%(asctime)s| %(message)s",
    datefmt="%I:%M",
    level=0,
)
logging.addLevelName(5, "UPDATE")
logging.Logger.update = _update


if __name__ == '__main__':
    logging.debug("a")
    logging.info("a")
    logging.error("a")
    # update and object are not defined on the module
    # However you can do something similar to defining it on the class.

    logger = logging.getLogger('foo_application')
    logger.debug("b")
    logger.info("b")
    logger.error("b")
    logger.update("b")
\$\endgroup\$
2
  • \$\begingroup\$ Monkey patching à la logging.Logger.update = _update does not play nice with mypy, unfortunately. \$\endgroup\$
    – Hyperplane
    Commented Dec 15, 2021 at 21:15
  • \$\begingroup\$ @Hyperplane Yes monkey patching with duck typing won't 'play nice' with static typing. However you can just # type: ignore or make an ILogger Protocol. \$\endgroup\$
    – Peilonrayz
    Commented Dec 15, 2021 at 21:26
5
\$\begingroup\$

Yes, this is not a good class design. You are mixing global state with in your methods and using it only as a namespace. Instead consider something like this:

from Time import Time
from datetime import datetime



class Logger:
    """If a log file for today already exist, open it in append mode.
    Else, create a new log file for today, and open it in append mode.
    """
    DEBUG = False
    PADDING = 9     # right-padding for type names in the log

    def __init__(self, filename=None):
        if filename is None:
            today = datetime.now().today()
            filename = f"logs/log{today.day}-{today.month}-{today.year}.txt"
        open(filename, "a").close()  # is this needed?

    def clear(self):
        """Clears the log file"""
        with open(self.filename, "r+") as file:
            file.truncate(0)

    def log(self, msg, level="INFO"):
        """Log at level"""
        with open(self.filename, "a") as file:
            type = level.ljust(self.PADDING)
            file.write(f"|{type}|{Time.getHour()}:{Time.getMinute()}| {msg}\n")

    def info(self, msg):
        """Log info"""
        self.log(msg, "INFO")

    def update(self, msg):
        """Used to log whenever a state is updated"""
        self.log(msg, "UPDATE")

    def debug(self, msg):
        """Only logs if the static variable {DEBUG} is set to True."""
        # Only log if DEBUG is set to True
        if not self.DEBUG:
            return
        self.log(msg, "DEBUG")

    def object(self, obj):
        """Intended for use on objects. They usually have a lot of information;
    therefor, we enclose the information with lines to make the log more readable."""
        with open(Logger.filename, "a") as file:
            file.write(f"-----------------------       object log\n")
            file.write(obj)
            file.write(f"\n-----------------------\n")

    def exception(self, msg):
        self.log(msg, "EXCEPTION")


if __name__ == "__main__":
    logger = Logger()
    logger.log("This is a test")
    logger.log("running something")
    logger.debug("Some debugging details")
    logger.exception("Critical error!")
Logger.debug("Some more debugging details")

Note the docstring conventions which require the string to be inside the methods and not shadowing the built-in object.

\$\endgroup\$
4
  • \$\begingroup\$ But if I don't HAVE to created instances of the Logger class, why should I? (as you did) \$\endgroup\$ Commented Jan 16, 2020 at 12:00
  • \$\begingroup\$ @SebastianNielsen: In that case you don't need a class at all, they can just be stand-alone functions in a separate module, since you are using the class only as a namespace (with the overhead of having to declare every method static. So instead of from logging import Logger; Logger.log("...") you do import logging; logging.log("..."). \$\endgroup\$
    – Graipher
    Commented Jan 16, 2020 at 12:55
  • \$\begingroup\$ @SebastianNielsen: Using your code also means that you can never have two loggers in two separate processes, they will always all write to the same file. \$\endgroup\$
    – Graipher
    Commented Jan 16, 2020 at 12:56
  • \$\begingroup\$ @SebastianNielsen: In other words, what's the point of a class if you never instantiate it or inherit from it? \$\endgroup\$
    – Graipher
    Commented Jan 16, 2020 at 12:59
2
\$\begingroup\$

Just to add to the other answers, not sure if you've considered this, but keep in mind that the file name will be the same unless you create a new instance of your class.

That means that if your logger instance is the same across midnight, you'll have the log of things that happened in one day written in the file of the previous day.

Maybe this is actually what you want? Again, not sure. But consider instead something along the lines of:

def get_log_filename():
    today = datetime.now().strftime(format="%d-%m-%Y")
    return f"logs/log{today}"

class Logger:
    def log(self, msg):
        with open(get_log_filename(), "a") as file:
            log_type = "INFO".ljust(Logger.PADDING)
            file.write("|{log_type}|{Time.getHour()}:{Time.getMinute()}| {msg}\n")

Another couple of things that I wanted to mention:

  • I understand wanting to put docstrings, but they should either be meaningful or not be there at all, otherwise it's just polluting the code. Having a method called clear with a docstring that says clears the logfile isn't really useful.

  • I'd also avoid naming a variable type, because not only it's very generic, there's also the built-in function type. The common practice when name clashes happen is to add an underscore at the end (type_), but in this case I think log_type is much better

\$\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.