Quoting robert engels (2018-11-25 00:15:21)
> 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)
>
> [...]
>
> A “senior developer” should look at this code and say, “wait the result is
> an objList, so it’s a List? Slice?
Having no idea where it's from and without looking anything up:
It's some sequence of objects. Given that ExtractList returns an error
it's probably being streamed from somewhere, though it's possible it's
already in memory and that error is a validation thing. ExtractList is
giving us back either a slice or a map, since we're seeing it used with
range below.
So I don't know what the concrete type of objList is, so what? The
intent of it being some kind of sequence (probably a stream of stuff being
fetched from a REST API) is entirely clear without that information. And
if I'm not already familiar with the APIs involved (which I'm not),
changing it to e.g:
var objList ResourceList
objList, err := ...
Does not give me any additional insight.
> 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.
The opacity of the code snippet above has more to do with this than it
does the absence of a completely uninformative type annotation. Type
inference is not the problem here. As a reviewer on a patch that added
this method, my approach here would be: skim the thing, make sure the
basic intent seems sane, mention any obvious problems, and ask the
submitter to write some comments, pick more helpful variable names,
then ping me and I'll do another pass.
> 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….
In this case the errors actually make a it a bit more clear what's going
on -- if there were no errors we'd assume a completely in-memory
operation where stuff can't fail.
Adding something like the check keyword from the draft design would be
the best of both worlds.
> 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…
Somewhat pedantic, but given the mention of dynamic languages, perhaps
relevant: there *is* a variable declaration -- it just doesn't include
the type. No worrying about where the variable comes from in the first
place.
> Reviewing Go code almost requires an IDE that pops the
> documentation/signatures up as you move over the code.
I find that sort of thing incredibly distracting. I've been using vim
with Go since the beginning; it's never been an issue for me (but then
I've never been much of an IDE user in general).
--
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.
