Take the 2-minute tour ×
Code Review Stack Exchange is a question and answer site for peer programmer code reviews. It's 100% free, no registration required.

Feel free to go get the package at:

bitbucket.org/agallego/log

or browse the code online.

Thanks for the code review.

share|improve this question
6  
Welcome to CodeReview! Would you please include (part of) your code in the question? –  S.L. Barth Oct 14 '12 at 7:40
add comment

closed as off-topic by codesparkle Aug 31 '13 at 1:46

This question appears to be off-topic. The users who voted to close gave this specific reason:

  • "Questions must include the code you want reviewed. Code must not appear only in a link to an external source. Doing so makes us dependent on a third party and makes it harder to review your code. If your code is very large, please select only the portions in which you are especially interested in a review. You are encouraged to keep the link to the rest of your code." – codesparkle
If this question can be reworded to fit the rules in the help center, please edit the question.

1 Answer

I reviewed your code and came up with a long list of comments.

I've incorporated most of the comments in the revised log.go and default.go (renamed from util.go) files.

log.go file

package log

import (
    "fmt"
    "io"
    "os"
    "sync"
    "time"
)

const (
    LevelDebug = 1 << iota
    LevelInfo
    LevelError
    LevelFatal
    LevelPanic
)

var levelText = map[int]string{
    LevelDebug: "DEBUG",
    LevelInfo:  "INFO ",
    LevelError: "ERROR",
    LevelFatal: "FATAL",
    LevelPanic: "PANIC",
}

type Logger struct {
    StampFormat string
    levels      int
    writer      io.Writer
    sync.Mutex
}

func New() *Logger {
    return &Logger{
        StampFormat: time.StampMilli,
        levels:      LevelInfo | LevelError | LevelFatal,
        writer:      os.Stdout,
    }
}

func (l *Logger) write(level int, format string, v ...interface{}) string {
    s := fmt.Sprintf(
        "[%s] %s %s\n",
        levelText[level],
        time.Now().Format(time.StampMilli),
        fmt.Sprintf(format, v...),
    )
    l.Lock()
    defer l.Unlock()
    n, err := l.writer.Write([]byte(s))
    if err != nil || n != len(s) {
        panic(
            fmt.Sprintf(
                "log.write failed: %d bytes to write, %d bytes written; %s",
                len(s), n, err,
            ),
        )
    }
    return s
}

func (l *Logger) Debug(format string, v ...interface{}) {
    if l.IsLevelSet(LevelDebug) {
        l.write(LevelDebug, format, v...)
    }
}

func (l *Logger) Error(format string, v ...interface{}) {
    if l.IsLevelSet(LevelError) {
        l.write(LevelError, format, v...)
    }
}

func (l *Logger) Info(format string, v ...interface{}) {
    if l.IsLevelSet(LevelInfo) {
        l.write(LevelInfo, format, v...)
    }
}

func (l *Logger) Fatal(format string, v ...interface{}) {
    if l.IsLevelSet(LevelFatal) {
        l.write(LevelFatal, format, v...)
        os.Exit(1)
    }
}

func (l *Logger) Panic(format string, v ...interface{}) {
    if l.IsLevelSet(LevelPanic) {
        panic(l.write(LevelPanic, format, v...))
    }
}

func (l *Logger) IsLevelSet(level int) bool {
    return (l.levels & level) == level
}

func (l *Logger) ToggleLevel(level int) {
    l.Lock()
    defer l.Unlock()
    if l.IsLevelSet(level) {
        l.levels &= ^level
    } else {
        l.levels |= level
    }
}

func (l *Logger) SetWriter(w io.Writer) {
    l.Lock()
    defer l.Unlock()
    l.writer = w
}

default.go file

package log

import (
    "io"
)

var defaultLogger = New()

func Debug(format string, v ...interface{}) {
    defaultLogger.Debug(format, v...)
}

func Info(format string, v ...interface{}) {
    defaultLogger.Info(format, v...)
}

func Error(format string, v ...interface{}) {
    defaultLogger.Error(format, v...)
}

func Fatal(format string, v ...interface{}) {
    defaultLogger.Fatal(format, v...)
}

func Panic(format string, v ...interface{}) {
    defaultLogger.Panic(format, v...)
}

func IsLevelSet(level int) bool {
    return defaultLogger.IsLevelSet(level)
}

func ToggleLevel(level int) {
    defaultLogger.ToggleLevel(level)
}

func SetWriter(w io.Writer) {
    defaultLogger.SetWriter(w)
}

There are three items of particular note.

First, in SetWriter you wrote,

// typecast and check if closer
if c, ok := l.writer.(interface {
    Close()
}); ok {
    c.Close()
}

In fact, interface { Close() } is not a Closer; interface { Close() error } is a Closer. Therefore, the value of ok is always false.

The correct code is,

if c, ok := l.writer.(io.Closer); ok {
    c.Close()
}

Now, you will discover that you are closing the WriterCloser os.Stdout, which means subsequent writes fail. The failure may not be obvious because you don't check for errors. My revised version checks for errors:

panic: log.write failed: 45 bytes to write, 0 bytes written; write /dev/stdout: bad file descriptor

Don't arbitrarily close a WriteCloser!

Second, you should use Go's godoc to document your log package.

Godoc: agallego/log

See: Godoc: documenting Go code.

For example,

Godoc: log

Godoc: log4go

Third, the Go standard library has a simple logging package named log.

share|improve this answer
add comment

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