4 ms·
The code example given seems a bit disingenuous, since the same level of complexity is present in the code, only the parts of it are broken up into single-purpo
by lux 15y ago
The code example given seems a bit disingenuous, since the same level of complexity is present in the code, only the parts of it are broken up into single-purpose methods that may or may not be overkill depending on the needs of the project. For example, you can swap `$this->file` with a different object to write to alternate logging backends, but you could just as easily replace `$this->logFile` with an alternate stream wrapper to do the same thing in the original example...
For reference, here's the first example:
public function log($message, $level) {
$f = fopen($this->logFile, 'a');
if (!$f) {
throw new LogicException('Could not open file for writing');
}
if (!flock($f, LOCK_EX | LOCK_NB)) {
throw new RuntimeException('Could not lock file');
}
fprintf($f, '%s [%s] %s - %s', $this->machineName, date('Y-m-d H:i:s'), $level, $message);
flock($f, LOCK_UN);
fclose($f);
}
And here's what your improved example would actually be:
/* In $this->file's class */
public function append ($message) {
$f = fopen($this->logFile, 'a');
if (!$f) {
throw new LogicException('Could not open file for writing');
}
if (!flock($f, LOCK_EX | LOCK_NB)) {
throw new RuntimeException('Could not lock file');
}
fwrite($f, $message);
flock($f, LOCK_UN);
fclose($f);
}
/* In Log class */
public function createLogMessage ($message, $level) {
return sprintf('%s [%s] %s - %s', $this->machineName, date('Y-m-d H:i:s'), $level, $message);
}
public function log($message, $level = 'notice') {
$message = $this->createLogMessage($message, $level);
$this->file->append($message);
}
Just a nitpick, but sometimes we over-abstract because we're too used to doing so. To me, a separate `createLogMessage()` method that just wraps a `sprintf()` seems like a sign of that.
(not saying I haven't been equally guilty of this 1000x before :)
- randallsquared 15y agoIf you're going to be creating log messages all over the place, that createLogMessage method is actually really useful. Not only does it enforce having the machine name and date (and date in a certain format), but calling it saves looking up what the argument to date has to be to get the format you want (which I nearly always have to do). It seems like an excellent case of hiding the fiddly details.
- lux 15y agoStill it doesn't necessarily do much more than the hard-coded "fprintf($f, '%s [%s] %s - %s', $this->machineName, date('Y-m-d H:i:s'), $level, $message);" already does in the original, unless you want to override it separately. I'd probably do something more like this: public $format = '%s [%s] %s - %s'; // machine, date, level, message public function formatMessage ($level, $message) { return sprintf ($this->format, $this->machine, date('Y-m-d H:i:s'), $level, $message); } public function log ($level, $message) { return $this->write ($this->formatMessage ($level, $message)); } That way you could change the format with no subclassing needed: $logger->format = '%s, %s, %s, %s'; I also like the idea of using a stream wrapper to override the logFile property: $logger->logFile = 'db://user:pass@localhost'; So again instead of subclassing, you just implement a stream wrapper as shown here: http://www.php.net/manual/en/stream.streamwrapper.example-1.php http://www.php.net/manual/en/stream.streamwrapper.example-1.... Perhaps that's just my personal aversion to needless depth in class hierarchy talking though... :) Edit: Wow does this thread have me geeking out! Haha, cheers :)
- randallsquared 15y agoI'm intrigued by the stream wrapper idea, but I don't think your "change the format" is general enough... sure, you can change the format, as long as you change it to something that uses the other arguments sprintf is being passed, but that's a pretty strong restriction. Maybe it would be workable if you had log() take optional argument(s) that represent the args to sprintf, too, but I'm not sure that it's worth it.
- lux 15y agoTrue, and using %1$s style references in the format in order to change their ordering is a lot uglier than overriding a method. In my last example, I did provide a formatMessage() which was the equivalent of the createLogMessage() for purposes of overriding. Best of both worlds :)
- jakejake 15y agoThe OP doesn't include the implementation of "createLogMessage" because he is suggesting that this method would be part of a framework, and by using it you can focus your mind on your application code, rather than boring details like checking for file locks on a log file. I do think that choosing where to abstract and where to split functionality is the "art" of the code and we obsess about as we master the craft. So I can appreciate your code example! I personally like to have a dedicated log function. That way you can refactor from using a file to perhaps a database table or any other method of logging. I personally expose our error logs as RSS and subscribe to the feed - I find it a really easy way to monitor because I'm too lazy to go looking at error log files every day!
- lux 15y agoI was assuming the log() method was also part of the framework, and that this was just a framework-level excerpt... ;) Perhaps such a core Log class could use a separate createLogMessage() method (or formatMessage() as I'd probably name it) that could be overridden in a subclass more easily than overriding the entire log() method. I definitely agree that the API design and finding that balance is a big part of what makes coding an artform!