Idiomatic Go Style notes

[nigeltao]

I just stumbled across this repo, linked to from golang/go#16353, which proposes to add packages under golang.org/x. A necessary but not sufficient condition for golang.org/x is that that code be in idiomatic Go style. Here are some miscellaneous notes on that topic, in no particular order:

First up, it doesn't build. AFAICT, the canonical github URL https://github.com/Comcast/gots has a capital C in Comcast. The import paths in the code are something like "github.com/comcast/gots" with a lower case C. I'm guessing that you're developing on Windows.

The example code in Readme.md doesn't look like it would build. The structure is:

func main() {
    etc
    if err == nil {
        etc
    } else {
        fmt.Printf(etc)
}

and you're missing an }.

Also, we usually check err != nil (and return early) instead of err == nil.

Also, the ".Error()" in

fmt.Printf("Unable to open file [%s] due to [%s]\n", filename, err.Error())

is redundant. Grep https://golang.org/pkg/fmt/ for "Error" for more details.

The loop:

pkt := make([]byte, packet.PacketSize)
for read, err := file.Read(pkt); read > 0 && err == nil; read, err = file.Read(pkt) {
    etc
}

is incorrect. As per https://golang.org/pkg/io/#Reader, the Read method is allowed to read fewer bytes than the buffer length. You may want https://golang.org/pkg/io/#ReadFull. You might also want a bufio.Reader to avoid lots of small reads.

In package packet:

return packet[0:start]

can be just

return packet[:start]

func badLen(packet Packet) bool {
    if len(packet) != PacketSize {
        return true
    }
    return false
}

can be just

func badLen(packet Packet) bool {
    return len(packet) != PacketSize
}

Similarly with func IsPat:

if pid(packet) == 0 {
    return true, nil
}
return false, nil

can be just:

return pid(packet) == 0, nil

The uint8 in

return packet[3] & uint8(0x0f), nil

is redundant, as per https://blog.golang.org/constants

Similarly, the int in

if int(packet[dataOffset+0]) == 0

is redundant.

Similarly for the uint8 in

func Length(packet packet.Packet) uint8 {
    return uint8(packet[4])
}

The parens in

return (packet[5] & 0x80) != 0

are unnecessary, and once you remove them, gofmt will emphasize the operator precedence:

return packet[5]&0x80 != 0

const PacketSize

can probably be just

const Size

as we are already in "package packet", so users of this constant would refer to it as "packet.Size". In the standard library's net/http package, the type is called Server, which users refer to as "http.Server". It's not "type HTTPServer" and "http.HTTPServer". Similarly for many other names in this repo.

Pid should probably be PID, as per https://github.com/golang/go/wiki/CodeReviewComments#initialisms, and similarly it'd be MPEG, AAC, CRC, etc.

"var emptyByteArray []byte" is unnecessary. Instead of:

return emptyByteArray, err

write:

return nil, err

and similarly, drop the "var emptyByteSlice []byte".

The "Array" in "emptyByteArray" is also incorrect: it's a slice, not an array. In Go, an array has fixed length.

In the _test.go files, the usual Go naming is "want", not "expected".

The outer parens in

!(bytes.Equal(payload, expected))

are unnecessary.

The byte in

packet[3] = byte(0x00)

is unnecessary.

The "== false" in:

if isNull, _ := IsNull(p); isNull == false {

is usually written:

if isNull, _ := IsNull(p); !isNull {

The "e" variable name in

pid, e := Pid(p)

should be "err".

The blank line in

// Create creates a new etc.
[blank line]
// Example usage
// etc
func Create(pid uint16, options ...func(*Packet)) Packet { etc }

means that godoc does not associate that first line comment line with the function. Instead, that blank line should be a blank comment line:

//

The asterisks in

func WithHasPayloadFlag(pkt *Packet) {
    (*pkt)[3] = byte((*pkt)[3] | 0x10)
}

are unnecessary. Packet is already a slice type, which contains an implicit pointer. There's no need for another indirection.

The second "size" in:

pay := make([]byte, size, size)

is redundant.

The double-parens in:

start := payloadStart((*pkt))

is unnecessary.

There is a typo with the "creatio" in "This is a convenience function for often used packet creatio options functions".

In func Create:

var pkt Packet = make([]byte, 188)

can be

pkt := make(Packet, 188)

Doc comments should also be complete sentences, and therefore end with a full stop. https://github.com/golang/go/wiki/CodeReviewComments#comment-sentences

That link also says that "Comments should begin with the name of the thing being described and end in a period", so that the "Parses" in

// Parses the accumulated packets and returns the
// concatenated payloads or any error that occurred, not both
func (a *accumulator) Parse() ([]byte, error) { etc }

should instead be

// Parse parses the accumulated packets and etc.

and similarly for various other doc comments in the repo.

I think that

func SetPayload(pkt *Packet, pay []byte) int {
    start := payloadStart((*pkt))
    i := start
    j := 0
    for i < PacketSize && j < len(pay) {
        (*pkt)[i] = pay[j]
        i++
        j++
    }
    return i - start
}

can instead be:

func SetPayload(pkt Packet, pay []byte) int {
    return copy(pkt[payloadStart(pkt):], pay)
}

if you are assuming that len(pkg) == PacketSize.

In general, it seems like functions like ContainsPayload, SetPayload and Equal could be methods instead of functions.

The make call in

if payloadUnitStartIndicator(pkt) {
    a.packets = make([]Packet, 0)
}

is probably redundant. A nil slice is a valid (empty) slice. It has length zero, you can range over it (zero times), and you can append to it.

This:

done, err := a.f(b)
if err != nil {
    return false, err
}
return done, nil

can probably just be

return a.f(b)

The var b line in

var b []byte
buf := bytes.NewBuffer(b)

is redundant, and the second can be just:

buf := bytes.NewBuffer(nil)

In package pmtdescriptor:

The receiver name, "descriptor" in

func (descriptor *pmtDescriptor) Tag() uint8 { etc }

is unusually long. See https://github.com/golang/go/wiki/CodeReviewComments#receiver-names

The constructor function:

func NewPmtDescriptor(tag uint8, data []byte) PmtDescriptor {
    descriptor := &pmtDescriptor{}
    descriptor.tag = tag
    descriptor.data = data
    return descriptor
}

could use a struct literal instead:

func NewPmtDescriptor(tag uint8, data []byte) PmtDescriptor {
    return &pmtDescriptor{
        tag:  tag,
        data: data,
    }
}

Go variable names lookLikeThis with camel case, not like representation_id_flag or num_partitions. https://golang.org/doc/effective_go.html#mixed-caps

indx is also an unusual variable name. Just use i.

The atscPmtStreamType type could be an anonymous struct. This:

type atscPmtStreamType struct {
    firstCode   uint8
    lastCode    uint8
    description string
}
var atscPmtStreamTypes = []atscPmtStreamType{etc}

could instead be

var atscPmtStreamTypes = []struct {
    firstCode   uint8
    lastCode    uint8
    description string
}{etc}

The i++ in

for i, descriptor := range descriptors {
    descriptorsBuf.WriteString(fmt.Sprintf("descriptor%d='%v'", i, descriptor))
    i++
    if i < len(descriptors) {
        descriptorsBuf.WriteString(",")
    }
}

looks like a bug.

Also,

descriptorsBuf.WriteString(fmt.Sprintf("descriptor%d='%v'", i, descriptor))

can instead be

fmt.Fprintf(&descriptorsBuf, "descriptor%d='%v'", i, descriptor)

The "descriptorsBuf" variable name is also unusually long, for such a short function (pmtElementaryStream.String).

This:

for i := 0; i < len(packets); i++ {
    pay, err := packet.Payload(packets[i])
    etc
}

can be:

for _, p := range packets {
    pay, err := packet.Payload(p)
    etc
}

or if you made Payload a method:

for _, p := range packets {
    pay, err := p.Payload()
    etc
}

but in the larger loop:

var pmtByteBuffer bytes.Buffer
for i := 0; i < len(packets); i++ {
    pay, err := packet.Payload(packets[i])
    if err != nil {
        return nil
    }
    pmtByteBuffer.Write(pay)
}

the bytes.Buffer is overkill. If you're just mushing byte slices together, have a "var buffer []byte" at the top and use the append function with dot-dot-dot.

The capital-B in

const BitsPerByte = 8

means that that constant is exported, but I doubt that users of that package would ever refer to psi.BitsPerByte. If that constant is only meant for internal use, the general rule is that we don't export names unless we have to. Similarly, it's not clear to me whether the psi.PointerField function needs to be exported or if it's only meant to be used by other functions inside that package.

There were more directories of code, but I didn't look at them. This note is already long enough.

I also only looked at code style in this note. I did not assess whether the API design style was idiomatic, which is again a necessary but not sufficient condition for moving under golang.org/x.

I hope that this has been helpful. Please let me know if it comes across as overly aggressive.

[nigeltao]

BTW in:

b[3] = byte(pts >> 7 & 0xff) // PTS[14..8]
b[4] = byte(pts&0xff) << 1 // PTS[7..0]

The 14..8 and 7..0 should be 14..7 and 6..0.

The &0xff is also redundant.

[guygrigsby]

Thank you! Your comments are certainly helpful. I didn't realize that there was already a conversation about x/media and this was mentioned. Cool.

[stilldavid]

A lot of these recommendations are pretty trivial, and I think useful to make the code more idiomatic and helpful for new users. If nobody else is working on cleanup, I'd be happy to help if it wouldn't be duplicating effort.

[guygrigsby]

Go for it. The only things we changed so far were the build issues. I just put the CLA in the repo too. (https://github.com/Comcast/gots/blob/master/CLA.pdf)

[onitake]

My 5¢:

Enum-like types like atscPmtStreamType shouldn't be structs, but simple types, and the PmtStreamType.StreamType() function should return the enum type and not uint8.

I'd suggest changing pmtstreamtype.go to the following:

type PmtStreamType uint8
const (
	PmtStreamTypeMpeg2VideoH262 PmtStreamType = 2  // H262
	PmtStreamTypeMpeg4Video = 27 // H264
        ...
	PmtStreamTypeIpmp = 25
        ...
)
func (t PmtStreamType) String() string {
        switch t {
        case PmtStreamTypeMpeg2VideoH262:
                return "ITU-T Rec. H.262 | ISO/IEC 13818-2 Video"
        case PmtStreamTypeMpeg4Video:
                return "AVC video stream as defined in ITU-T Rec. H.264 | ISO/IEC 14496-10 Video"
        ...
        }
        // optional
        if t >= 196 && t <= 233 {
                return "TSC User Private Program Elements"
        }
        return fmt.Sprintf("0x%02x", int(t))
}
type PmtStreamType interface {
	StreamType() PmtStreamType
}

This will make it much more comfortable to use.

And I'd do the same for pmtDescriptor.tag, adding a public enum type for it with a String() method.