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:
can be just
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
can probably be just
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:
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
is unnecessary.
The "== false" in:
if isNull, _ := IsNull(p); isNull == false {
is usually written:
if isNull, _ := IsNull(p); !isNull {
The "e" variable name in
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
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
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
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.
React
Typescript
Django
Laravel
Facebook
Microsoft
Google
Alibaba
D3
Tencent