You kind of made my point, when you state “it’s either a slice or a map”… Well,
what if the resource needed to be ordered, which is why the previous method has
List in its name, but the method was actually returning a map. Yes, someone
should of caught that in the code review for the extractList method, but if
someone didn’t, the error is being compounded here, with no obvious way to
detect that during a code review…
Errors pile up, especially design ones, when types get omitted - people start
forgetting that the types often enforce the design, especially when dealing
with data structures.
> On Nov 24, 2018, at 11:58 PM, Ian Denhardt <i...@zenhack.net> wrote:
>
> 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.
--
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.
