Is it idiomatic to return a badly structured object together with error?

agolangf · · 400 次点击

这是一个分享于的资源，其中的信息可能已经有所发展或是发生改变。

Suppose I have a method that does some work and than returns the result and an error: <pre><code>type Res struct { // some fields } func foo() (Res, error) { res := Res{ // some initialization } err := DoSomeWork(&res) if err != nil { return nil, err } return res, err } </code></pre> Let us assume res is modified in DoSomeWork, and is unusable before it executes. It seems redundant to do the err check explicitly, and instead I could have done something like this: <pre><code>func foo() (Res, error) { res := Res{ // some initialization } err := DoSomeWork(&res) return res, err } </code></pre> Than if there was an error, err will not be nil, and as long as whoever calls foo checks for the error, everything is fine. Would this be idiomatic code, or should I explicitly check for errors, so that nobody gets an unusable object returned? <hr/>**评论：** ofux: <pre>It is perfectly fine to return an object in an undefined/bad state together with an error. It is up to the caller to check if there is an error (ie. if the error is nil or not). Readability is more important here. So second solution is better in your case. However, never do the same with the error itself. In other words, never return a badly defined error in case there is no error. An error must be either nil or well defined. Be sure to read this about this point: <a href="https://golang.org/doc/faq#nil_error">https://golang.org/doc/faq#nil_error</a></pre>peterbourgon: <pre>I tend to write things the first way just to be safe, but there's nothing wrong with the second way. If we're code golfing, this is also something: <pre><code>func NewFoo() (*Foo, error) { f := &Foo{ // ... } return f, doSomeWork(f) } </code></pre></pre>ofux: <pre>I personally don't like this: <pre><code>return f, doSomeWork(f) </code></pre> Just a matter of readability again. Even if we all know that doSomeWork will be called before the return statement gets executed, it is less obvious than <pre><code>err := DoSomeWork(&res) return res, err </code></pre></pre>ian-kent: <pre>I'd go with your second example - I personally find it far more readable, and a lot of libraries (including core packages afaik) use that pattern, where the struct is returned by shouldn't be trusted</pre>natefinch: <pre>IMO, it's best practice to explicitly return a zero value for the "other value" if you're returning a non-nil error. In general, it should be fine to return bad data, because people should be checking your error before they use the value. However, I always return a zero value instead, because it makes using your code less error prone. Consider if someone does this: <pre><code>val.Something, err = yourFunc() </code></pre> Now, maybe they check err here and exit early... but they've saved that bad value in a struct somewhere, and it may come back to bite them later in the code.</pre>G00PHR: <pre>That is bad design. Assign the return values to temp variables and check the error, if the error is non-nil, assign the temp to its permanent variable within the struct. Assigning random and possibly incorrect values to fields then checking if there was an error, and if there was an error reassigning that value is just silly. What is the zero value of an 'int'? that's right - ZERO. Zero is a valid value of int. What if zero is also a valid return? You check your errors. This is why if the func returns an error that you should check the error. This is also the reason why you should not rely on returning zero values with errors. Zeroing out could create unnecessary overhead. If the error is non-nil then the values returned with the error should be considered invalid, period. </pre>natefinch: <pre>I don't disagree with you. I'm not saying you should assign things to struct fields without checking the error first. But I'm saying that you can potentially help people debug problems if you return a zero value. Yes, a zero value may well be valid, but it's still the zero value, and it still gives the developer some indication that maybe there was a problem creating the value, if they were expecting it to be something else. You should definitely not worry about the overhead of returning a zero value instead of a half-populated value until such a time as you know it is a bottleneck.... and I would guess that an average developer would never see a time when that's a bottleneck.</pre>G00PHR: <pre>If there is an error then the value returned is invalid, so why worry about returning a zero value with the error, this makes even more sense when there is a possibility that the zero value is also a valid value. You are creating unnecessary edge cases. If error is non-nil then the returned values with the error should be considered invalid, period. <blockquote> and I would guess that an average developer would never see a time when that's a bottleneck. </blockquote> so you initialize a big, huge, colossal struct, then pass it into a nested func, and that func returns an error with this invalid big, huge, colossal struct, so you then initialize a new big, huge, colossal struct with zero values and return that with the error, and you don't guess there would ever be a time where that could be seen as a bottleneck? Outrageous!</pre>TheMerovius: <pre><blockquote> If there is an error then the value returned is invalid, so why worry about returning a zero value with the error, this makes even more sense when there is a possibility that the zero value is also a valid value. You are creating unnecessary edge cases. If error is non-nil then the returned values with the error should be considered invalid, period. </blockquote> No one argues that this should be. Just that it won't be that way. There will always be people who forget. And yes, you could then say "well, who cares, not my bug", that is your prerogative. But some people (myself included) elect to make other people life simpler, even if it was their mistakes and not mine.</pre>G00PHR: <pre>Yes, more edge cases are simpler.... Instead of just checking the error. Seriously, you aren't making anything simpler. You are actually going against the grain, creating more edge cases. Zero values are valid values. I already gave a super simple case that could be expanded to most types. Just check your errors. You are preaching in direct opposition of the core go team with regard to zero values and errors.</pre>TheMerovius: <pre><blockquote> Instead of just checking the error. </blockquote> You still seem to not understand that we are talking about code I do not control. <blockquote> Zero values are valid values. </blockquote> Neither the zero value of os.File, nor of *os.File are valid values and they could never be made that way. Blanket statements like this are never true. <blockquote> Just check your errors. </blockquote> I explicitly said I do, I do not understand why you keep stating that this would be an appropriate thing to say. <blockquote> ou are preaching in direct opposition of the core go team with regard to zero values and errors. </blockquote> Can you give me an example of where the go team said that it is wrong to pay attention to either return a valid value or a non-nil error? Because I have never heard or read anything like that. The general consensus™ is: a) If possible, make the zero value useful b) If that's not possible, provide a NewT function c) Do not use any return value before checking for a non-nil error. And, to re-emphasize this again: I do not disagree with a single one of these rules at all. I'm simply adding, for my personal taste: d) If you can't return a valid value, don't return something that could be mistaken for it I do this, because I am aware that not everyone will adhere to Rule c above, intentionally or not. And if they don't, I want to make it easier for them to spot their mistake. Because it doesn't cost me anything, really, and "well, it's your own fault, because you just shouldn't be fallible" just isn't a thing I am comfortable telling people or to make part of my world view. If you do, that's fine.</pre>G00PHR: <pre>okay, done here. You clearly haven't got a clue. It is real simple, yet you can't seem to grasp it. Zero values are useful, zero values are valid values, you should always check errors and if the error is non-nil then the rest of the returned values should be considered invalid. Spreading the notion that zeroing a value could help find problems is absurd, and creates unnecessary edge cases. Your advice will create a wave of inept, backwards thinking Go devs. I never said all values are valid all the time. I gave you the notion that zero values are useful and valid, so why rely on the returned values without checking the errors. You are creating unnecessary edge cases, when you could just check the error. You are reading into something that is not there. And why waste time zeroing out returns if you are returning an error. rob pike and others have speckled their talks with these key points. Spark up a thread on go-nuts with this subject and I'm sure they will reiterate on the topics for you. I can't talk some sense into you but maybe they can.</pre>TheMerovius: <pre>Thanks for rounding my already good evening with another smile by again just completely ignoring what I wrote :)</pre>TheMerovius: <pre><blockquote> Spark up a thread on go-nuts with this subject and I'm sure they will reiterate on the topics for you. I can't talk some sense into you but maybe they can. </blockquote> <a href="https://groups.google.com/forum/#!topic/golang-nuts/lcZNt-nMsl0" rel="nofollow">Feel free to chime in</a>. I'm nothing if not open for arguments and having my mind changed.</pre>dgryski: <pre>More likely that this is unlikely to be happening in a tight loop where performance is important rather than there is no performance cost at all.</pre>G00PHR: <pre>Initialization of variables can happen anywhere, and it doesn't matter where it happens - so you aren't making any sense. I don't care if it happens just once in a program, there is no need for it. That pattern initializes the same struct twice for no reason, yet you justify its use. This sub needs new top mods. This answer you gave to help your fellow top mod to save face is just as stupid of an answer he gave - if not stupider.</pre>tscs37: <pre>If there is an error then unless the documentation explicitly states otherwise, everything else about the function return data is in a bad or corrupted or undefined state. For simplicity, I do that too, there is no rational programmer that should be using the results of an errored functions for very very obvious reasons. The exception is when you have special errors that aren't actually errors but indicate some state or special case of the execution.</pre>mcandre: <pre>The safest thing to do is return a nil pointer for the struct, along with a meaningful error trace. But it's not strictly necessary to return a wholesome data structure if your users can be assumed to run the error check.</pre>shovelpost: <pre>If you run the <code>gosimple</code> linter alone or as part of <code>gometalinter</code> it will probably suggest you that the first version can be simplified to the second version. I am not sure how "idiomatic" it is because <code>gosimple</code> is not part of the official tooling but as others have mentioned here it is the responsibility of the caller to check for the error.</pre>acron5n5: <pre>It is ok but better return nil/empty object, just less confusion and maybe less GC work in some cases.</pre>TheMerovius: <pre>I consider it bad style. To me, it's basically defense in depth. You having a new-function already means, that you care about it having correct initialization state; by explicitly returning nil, you make sure that a user can not make a mistake here. So, my approach is a) always check for err before using returns and b) always make sure, that even if a) is forgotten, the program will just instantly crash.</pre>