I whole heartily agree, and for “tight” systems-level code, with limited
function APIs (like err), it works fine. The example you cite is trivial, and a
“well known” API due to its common usage.
Contrast that with this actual code from the leading Go application (picked at
random):
func (p *pruner) prune(namespace string, mapping *meta.RESTMapping,
includeUninitialized bool) error {
objList, err := p.dynamicClient.Resource(mapping.Resource).
Namespace(namespace).
List(metav1.ListOptions{
LabelSelector: p.labelSelector,
FieldSelector: p.fieldSelector,
IncludeUninitialized: includeUninitialized,
})
if err != nil {
return err
}
objs, err := meta.ExtractList(objList)
if err != nil {
return err
}
for _, obj := range objs {
metadata, err := meta.Accessor(obj)
if err != nil {
return err
}
You have no way to understand what objList is without going to the source/doc
of the method, and then it might even be an interface… (hopefully a well
designed one)
At least one extra level of indirection into the documentation is always
required.
A “senior developer” should look at this code and say, “wait the result is an
objList, so it’s a List? Slice?, no, then I need to meta to extract a List, so
am I getting a slice of something meta, or am I just extracting a slice from a
collection so I can use range…” Not to mention that there is not a single
comment line describing what a 50+ line method should do - with a generic name
like prune()… maybe with an overview of the purpose, the lack of internal
documentation might be passable.
Not having the actual types here, and vars with names like objList, then objs,
then obj is not “no surprises" development.
It’s horrible, and anyone with any experience knows this is the case… Reviewing
this code blind - you need to do a lot of mental gymnastics to “figure out the
api”. You need to be deeply familiar with a huge API to develop any sort of
understanding.
With non-systems applications, i.e. business level, or macro/functional apps,
the APIs are much more involved and missing type information is a real problem
IMO.
And as an aside, any “noise removed” in this case, is certainly added back by
the “if err != nil” noise, instead of proper exceptions. This is a classic case
that leads to... “well we don’t really code review, if the test cases pass, it
must be good”. Don’t want to be the next guy….
I know its a balance, and this is a “private” method, so there is leeway there,
but I think not having declared variables in the code hurts maintainability -
the only thing being worse are the dynamic languages where all hell breaks
loose…
Reviewing Go code almost requires an IDE that pops the documentation/signatures
up as you move over the code.
> On Nov 24, 2018, at 7:45 PM, Ian Denhardt <i...@zenhack.net> wrote:
>
> Quoting robert engels (2018-11-24 17:34:34)
>> Agreed, but that is why I have a big problem with variable inference as used
>> in Go (and now Java 9 with var).
>>
>> You need to refer to the callee API doc in order to gain understanding of
>> the code - as the type information is not readily available. Even though
>> there are less characters, so people claim - easier to read - the loss of
>> information when dealing with non-common methods is a huge loss in
>> readability/understanding IMO.
>
> Having spent a lot bit of time working in languages which don't require
> writing as many types, either because they can infer them (like Go, or
> to a much greater extent ML-family languages), or because they just
> don't have them (Python), I've found well-designed APIs make the absence
> of this information at the call site not much of a problem. Designing
> for the call site is a good idea in any language. What that entails may
> vary based on the details of the language itself. It always at least
> involves good names; errors.New is pretty self explanatory. This:
>
> var err error = errors.New(...)
>
> ..is just noise.
>
>> I find myself continually reviewing the documentation when reviewing
>> Go code - much more so than Java - with external frameworks, etc.
>
> If I'm doing code review and I have a hard time understanding it, I usually
> ask the author to fix the code.
>
> If you have control over the callee API it can take the form of improving
> that, so that the call site is clearer. Sometimes a simple comment will help.
>
> Sometimes that means just putting the type annotation there even though you
> don't have to. This is common in ML family languages, where type annotations
> are never actually required *anywhere*. All language features are abusable.
>
> -Ian
>
> --
> You received this message because you are subscribed to the Google Groups
> "golang-nuts" group.
> To unsubscribe from this group and stop receiving emails from it, send an
> email to golang-nuts+unsubscr...@googlegroups.com.
> For more options, visit https://groups.google.com/d/optout.
--
You received this message because you are subscribed to the Google Groups
"golang-nuts" group.
To unsubscribe from this group and stop receiving emails from it, send an email
to golang-nuts+unsubscr...@googlegroups.com.
For more options, visit https://groups.google.com/d/optout.
