FAQ
This is my first released bit of Go code

https://github.com/ncw/stressdisk

it is a utility for stressing disk drives to check them out before you
trust your data to them.

The original was a C program with a Perl driving script - I re-wrote it
in Go so I could have fast and easy in the same language! Sticking some
goroutines in made it faster too.

If anyone would like to comment on the code I would be grateful

Thanks

Nick
--
Nick Craig-Wood <nick@craig-wood.com> -- http://www.craig-wood.com/nick

--

Search Discussions

  • Yves Junqueira at Sep 22, 2012 at 10:05 am
    Nick, thank you for releasing this useful tool as open source. I'll
    give it a try later on my raspberryPi.

    I have a few style suggestions, but this is not a complete review yet.
    Only after writing this email I realized it would have been easier to
    add the comments to github directly. I'll do that in the next round of
    reviews.

    constants like SIZE are better named as Size. This style wasn't
    followed in early Go programs, but it's now more common (see the time
    package for example)

    functions like read_file() should be named as readFile().

    variables like read_speed should be named readSpeed.

    The "const MB" declaration should be at the package level.

    Don't call panic in outputDiff(). Use errors instead.

    Calling os.Exit(0) in initialiseStats() is a bit weird. Can't you use
    the 'finished' channel instead?

    Your doc comments don't follow the usual style. See
    http://golang.org/doc/effective_go.html#commentary. Specifically "Doc
    comments work best as complete sentences, which allow a wide variety
    of automated presentations. The first sentence should be a
    one-sentence summary that starts with the name being declared."

    Your single-letter flags (w, r, R) are, in my opinion, abusing the
    flags system. They are command names, not flags, so they should be
    positional arguments. Use flags.Args() to look at positional
    arguments. The "go" program command line interface is an excellent
    example. A user runs:

    go fmt -x <package>

    Instead of:

    go -fmt -x <package>

    Underneath, the go tool commands are pluggable structs with attached
    functions, which looks quite nice in code. See:
    http://golang.org/src/cmd/go/main.go

    This package should help writing command line tools using the go tool
    style: http://code.google.com/p/go-commander/. It needs better
    documentation, but could be useful already.

    On Sat, Sep 22, 2012 at 10:10 AM, Nick Craig-Wood wrote:
    This is my first released bit of Go code

    https://github.com/ncw/stressdisk

    it is a utility for stressing disk drives to check them out before you
    trust your data to them.

    The original was a C program with a Perl driving script - I re-wrote it
    in Go so I could have fast and easy in the same language! Sticking some
    goroutines in made it faster too.

    If anyone would like to comment on the code I would be grateful

    Thanks

    Nick
    --
    Nick Craig-Wood <nick@craig-wood.com> -- http://www.craig-wood.com/nick

    --


    --
    Yves Junqueira <http://cetico.org/about>

    --
  • Nick Craig-Wood at Sep 22, 2012 at 10:21 am

    On 22/09/12 11:05, Yves Junqueira wrote:
    Nick, thank you for releasing this useful tool as open source. I'll
    give it a try later on my raspberryPi. Grand!
    I have a few style suggestions, but this is not a complete review yet.
    Only after writing this email I realized it would have been easier to
    add the comments to github directly. I'll do that in the next round of
    reviews.

    constants like SIZE are better named as Size. This style wasn't
    followed in early Go programs, but it's now more common (see the time
    package for example)

    functions like read_file() should be named as readFile().

    variables like read_speed should be named readSpeed.
    Yes good catch. These are left overs from the C port.
    The "const MB" declaration should be at the package level. Yup
    Don't call panic in outputDiff(). Use errors instead.
    I'd have to disagree with you on that. The panic is there to catch a
    programmer error rather than anything else - it should never happen.
    Calling os.Exit(0) in initialiseStats() is a bit weird. Can't you use
    the 'finished' channel instead?
    Yes I think you are right - it is a bit weird
    Your doc comments don't follow the usual style. See
    http://golang.org/doc/effective_go.html#commentary. Specifically "Doc
    comments work best as complete sentences, which allow a wide variety
    of automated presentations. The first sentence should be a
    one-sentence summary that starts with the name being declared." OK
    Your single-letter flags (w, r, R) are, in my opinion, abusing the
    flags system. They are command names, not flags, so they should be
    positional arguments.
    Yes you are absolutely right. In the original C program they were
    single letter commands but I got a bit carried away with the goodness of
    the flags package!
    Use flags.Args() to look at positional
    arguments. The "go" program command line interface is an excellent
    example. A user runs:

    go fmt -x <package>

    Instead of:

    go -fmt -x <package>

    Underneath, the go tool commands are pluggable structs with attached
    functions, which looks quite nice in code. See:
    http://golang.org/src/cmd/go/main.go
    A very good suggestion - thank you.
    This package should help writing command line tools using the go tool
    style: http://code.google.com/p/go-commander/. It needs better
    documentation, but could be useful already.
    OK I'll give it a go.

    Thanks for taking the time to do a review and I'll be updating the code
    with your points shortly!

    Thanks

    Nick

    --
    Nick Craig-Wood <nick@craig-wood.com> -- http://www.craig-wood.com/nick

    --
  • Yves Junqueira at Sep 22, 2012 at 10:33 am

    On Sat, Sep 22, 2012 at 12:21 PM, Nick Craig-Wood wrote:
    On 22/09/12 11:05, Yves Junqueira wrote:
    Nick, thank you for releasing this useful tool as open source. I'll
    give it a try later on my raspberryPi. Grand!
    I have a few style suggestions, but this is not a complete review yet.
    Only after writing this email I realized it would have been easier to
    add the comments to github directly. I'll do that in the next round of
    reviews.

    constants like SIZE are better named as Size. This style wasn't
    followed in early Go programs, but it's now more common (see the time
    package for example)

    functions like read_file() should be named as readFile().

    variables like read_speed should be named readSpeed.
    Yes good catch. These are left overs from the C port.
    The "const MB" declaration should be at the package level. Yup
    Don't call panic in outputDiff(). Use errors instead.
    I'd have to disagree with you on that. The panic is there to catch a
    programmer error rather than anything else - it should never happen.
    That's reasonable. But if stressdisk ever becomes a library, you
    should definitely use an error instead of a panic.
    Calling os.Exit(0) in initialiseStats() is a bit weird. Can't you use
    the 'finished' channel instead?
    Yes I think you are right - it is a bit weird
    Your doc comments don't follow the usual style. See
    http://golang.org/doc/effective_go.html#commentary. Specifically "Doc
    comments work best as complete sentences, which allow a wide variety
    of automated presentations. The first sentence should be a
    one-sentence summary that starts with the name being declared." OK
    Your single-letter flags (w, r, R) are, in my opinion, abusing the
    flags system. They are command names, not flags, so they should be
    positional arguments.
    Yes you are absolutely right. In the original C program they were
    single letter commands but I got a bit carried away with the goodness of
    the flags package!
    Use flags.Args() to look at positional
    arguments. The "go" program command line interface is an excellent
    example. A user runs:

    go fmt -x <package>

    Instead of:

    go -fmt -x <package>

    Underneath, the go tool commands are pluggable structs with attached
    functions, which looks quite nice in code. See:
    http://golang.org/src/cmd/go/main.go
    A very good suggestion - thank you.
    This package should help writing command line tools using the go tool
    style: http://code.google.com/p/go-commander/. It needs better
    documentation, but could be useful already.
    OK I'll give it a go.

    Thanks for taking the time to do a review and I'll be updating the code
    with your points shortly!

    Thanks

    Nick

    --
    Nick Craig-Wood <nick@craig-wood.com> -- http://www.craig-wood.com/nick

    --


    --
    Yves Junqueira <http://cetico.org/about>

    --
  • Jesse McNelis at Sep 22, 2012 at 10:46 am

    On Sat, Sep 22, 2012 at 8:32 PM, Yves Junqueira wrote:
    On Sat, Sep 22, 2012 at 12:21 PM, Nick Craig-Wood wrote:
    I'd have to disagree with you on that. The panic is there to catch a
    programmer error rather than anything else - it should never happen.
    That's reasonable. But if stressdisk ever becomes a library, you
    should definitely use an error instead of a panic.
    What would you do with that error?
    How would you handle it?

    --
    =====================
    http://jessta.id.au

    --
  • Yves Junqueira at Sep 22, 2012 at 11:09 am

    On Sat, Sep 22, 2012 at 12:46 PM, Jesse McNelis wrote:
    On Sat, Sep 22, 2012 at 8:32 PM, Yves Junqueira
    wrote:
    On Sat, Sep 22, 2012 at 12:21 PM, Nick Craig-Wood wrote:
    I'd have to disagree with you on that. The panic is there to catch a
    programmer error rather than anything else - it should never happen.
    That's reasonable. But if stressdisk ever becomes a library, you
    should definitely use an error instead of a panic.
    What would you do with that error?
    How would you handle it?
    Potentially, my program could only pass an incorrect input to
    stressdisk in corner cases, and maybe my program wants to continue
    running if the stressdisk operation failed. Using errors makes this
    considerably easier.

    Other programmers my think differently ("a panic would make the
    problem more obvious"), but in any case, panic in libraries should be
    documented ("Passing a and b of different lengths causes a run time
    panic").

    All this said, a closer inspection of stressdisk shows that this panic
    is not likely to happen based on user input and system behavior, but
    really only because of a programmer error on stressdisk's side, as
    Nick said, so my suggestion is probably not really relevant.

    --
    Yves Junqueira <http://cetico.org/about>

    --

Related Discussions

Discussion Navigation
viewthread | post
Discussion Overview
groupgolang-nuts @
categoriesgo
postedSep 22, '12 at 8:11a
activeSep 22, '12 at 11:09a
posts6
users3
websitegolang.org

People

Translate

site design / logo © 2022 Grokbase