search

m13253 / dns-over-https

High performance DNS over HTTPS client & server
https://developers.google.com/speed/public-dns/docs/dns-over-https
MIT License
1.96k stars 221 forks source link

Simplify doDNSQuery call #110

Closed gdm85 closed 3 years ago

gdm85 commented 3 years ago

Simplify doDNSQuery call: there is an extra return value which is unnecessary.

In fact, if you agree with #108 we could add some linter step.

m13253 commented 3 years ago

This return value is deliberately added. Think of:

s := []int{1, 2, 3}
s = append(s, 42)

The function is consuming something, processing it, then outputs the same thing again.

Since Golang doesn't have a way to mark side effects, this is a trick to leave for future extension without changing the API. (e.g. the function can output other thing instead of modifying existing thing in the future.)

Maybe the linter complains about this. But I am sure about what the extra return value means. :-)

gdm85 commented 3 years ago

Maybe the linter complains about this.

Have not tried yet running it, do not know.

But I am sure about what the extra return value means. :-)

That is the problem, it is not idiomatic Go IMO and thus others will not get the same meaning/value out of it :)

If you want to follow up on the aspect you mentioned about state-changing, the code should be changed to:

That way nothing is changed from the passed req object.

m13253 commented 3 years ago

I see... It's not idiomatic Go, more similar to some functional programming style.

Do you have any better suggestions than just remove it? Maybe make a deep clone + write the response + return it?

gdm85 commented 3 years ago

I see... It's not idiomatic Go, more similar to some functional programming style.

Do not take my word for it...I have simply not seen this in the Go stdlib or other big Go projects, your experience might differ.

I think the reader expectation here is that the returned resp can be a totally different instance than req passed via arguments.

Passing the req as a solid struct instead of a pointer would be more correct, but inefficient.

I would rather look at the DNSRequest and at this function doDNSQuery: they should both try to do one thing and do it well, while at the moment they are trying to do multiple things. For example the request contains the response as well, and I am not sure the request needs to track the current upstream (they are randomly selected, so does not matter at which level you account for the current one; it would be different if the already-tried upstreams were removed from a slice).

By simplifying DNSRequest you could probably get a simpler doDNSQuery that acts more as an input -> output function where the input is the request and the output is the response, and side effects - if any - are partitioned to the server object (changing the current upstream).

But it is not a big deal - feel free to close this PR :)

m13253 commented 3 years ago

I think the reader expectation here is that the returned resp can be a totally different instance than req passed via arguments.

Haha, this was true in the past ... until at some point I need the original request to do some workarounds at later stage of the pipeline. Then resp becomes the same as req.

I can merge this pull request to prevent confusion. I think I will redesign the whole thing sometime in the future.

m13253 commented 2 years ago

Thanks for being a frequent contributor. I'm starting to think of inviting more people to help me with the maintenance, do you mind if I invite you to the write access group of this repo?

gdm85 commented 2 years ago

Sure, I will do my best to help!

m13253 commented 2 years ago

Sure, I will do my best to help!

Check your email for an invitation.