on goto
in [General Programming]
|
From: Nick on 30 Apr 2010 01:46 Keith Thompson <kst-u(a)mib.org> writes: > Nick <3-nospam(a)temporary-address.org.uk> writes: >> Keith Thompson <kst-u(a)mib.org> writes: >>> Jeff Flinn <TriumphSprint2000(a)hotmail.com> writes: >>>> Keith Thompson wrote: >>>>> Jeff Flinn <TriumphSprint2000(a)hotmail.com> writes: >>>>>> Keith Thompson wrote: >>>>>>> Seebs <usenet-nospam(a)seebs.net> writes: >>>>>>>> On 2010-04-28, bart.c <bartc(a)freeuk.com> wrote: >>>>>>>>> int* findvalue(int value) >>>>>>>>> {int *a=0; >>>>>>>>> >>>>>>>>> for(int xInd = 0; !a && xInd < datasize; ++xInd) >>>>>>>>> for(int yInd = 0; !a && yInd < dataxindsize; ++yInd) >>>>>>>>> for(int zInd = 0; !a && zInd < dataxindyindsize; ++zInd) >>>>>>>>> { >>>>>>>>> if(data[xInd][yInd][zInd] == value) >>>>>>>>> a=&data[xInd][yInd][zInd]; >>>>>>>>> } >>>>>>>>> >>>>>>>>> return a; >>>>>>>>> } >>>>>>>> This is noticably slower and less clear. I would not consider it >>>>>>>> an acceptable rewrite -- but I would consider the other form a good >>>>>>>> rewrite of it. >>>>>>> I mostly agree, but -- "noticably slower"? How did you notice? >>>>>> Certainly is for the case data[0][0][0] == value. ;-) >>>>> >>>>> How so? >>>>> >>>>> I'm not arguing that it's not *likely* to be slower, but (a) it's not >>>>> likely to be *noticeably* slower unless the entire loop is executed many >>>>> many times, and (b) it might not be slower at all, depending on how >>>>> clever the compiler is. >>>>> >>>>> (I'm assuming there's not some semantic difference that I haven't >>>>> noticed.) >>>> >>>> Have you profiled your 'assign address' version versus the previous >>>> 'return when found' version? For different sizes? The 'assign address' >>>> version is effectively doing a std::for_each, alway visiting every >>>> element even if the item found is the 1st one. The 'return when found' >>>> version is analogous to std::find_if. For the case data[0][0][0] == >>>> value, your algorithm is O(N), while find_if, is O(1). >>> >>> Please look again. I'm fairly sure that all the versions I've seen in >>> this thread terminate after finding the item. >> >> Not when the item is zero, because that's the "nothing has been found >> yet" flag, so the search executes to exhaustion even if the item was >> found hours ago. > > No, the loops test for ``!a''. ``a'' is a pointer, initialized to 0 > (NULL). ``data'' is presumably a 3-D array of int. When the item > is found, a is set to point to the item; even if the item's value > is 0, a is still non-zero (non-NULL). Bloody hell, so it is. So I want to look for an integer, so I create a pointer to look for it, just so that I can I use null as a distinct signal value. Of course, "a" was a particularly carefully chosen name to make this clear. And that's the better and clearer code? Yeah, right! -- Online waterways route planner | http://canalplan.eu Plan trips, see photos, check facilities | http://canalplan.org.uk
From: tonydee on 30 Apr 2010 04:58 On Apr 29, 8:51 pm, "Daniel T." <danie...(a)earthlink.net> wrote: > Nick Keighley <nick_keighley_nos...(a)hotmail.com> wrote: > > On 28 Apr, 13:59, "Daniel T." <danie...(a)earthlink.net> wrote: > > > SG <s.gesem...(a)gmail.com> wrote: > > > > On 28 Apr., 11:02, Richard Heathfield wrote: > > > > Value_t* MyClass::findValue(const Value_t& value) > > > > { > > > > Value_t* result = 0; > > > > for(size_t xInd = 0; !result && xInd < data.size(); ++xInd) > > > > for(size_t yInd = 0; !result && yInd < data[xInd].size(); > > > > ++yInd) > > > > for(size_t zInd = 0; !result && zInd < data[xInd] > > > > [yInd].size(); > > > > ++zInd) > > > > { > > > > if(data[xInd][yInd][zInd] == value) > > > > result = &data[xInd][yInd][zInd]; > > > > } > > > > return result; > > > > } > > > > > I don't know about you but I like the first version better. It's more > > > > concise. I find it easier to see what the loop's doing. Maybe it's > > > > just me. I guess I'm used to these kinds of loops. > > > > Since the sample code is obviously in c++, I would rather see something > > > like: > > > > Iterator it = data.begin() > > > while(it != data.end() && *it != value) > > > ++it; > > > return it != data.end(); > [snip] > The point of my example was to show that the problem with Juha's code > wasn't that it had multiple exits, but rather that it was at the wrong > level of abstraction and therefore seemed to need multiple exits. > Richard, removed the multiple exits without fixing the abstraction > problem and he ended up with a worse result. That's a very insightful point, and in many ways good practice - particularly if it prevents client code coupling to the implementation or layout of your container. In this case though, the find function might be a member of the container - inside the encapsulation - and coupling quite acceptable. Further, say you want something just marginally more complicated - like skipping every second value of yInd: you can't do that with your iterator (the x/y/z boundaries are lost), but it would be a trivial modification to the explicit loop. Localisation, concision, directness and simplicity must be balanced against abstraction - the latter is not always better. So, while I agree an iterator may sometimes be good, I strongly disagree that the explicitly nested loops were inherently at the wrong level of abstraction, yielding a necessarily worse result. Cheers, Tony
From: bart.c on 30 Apr 2010 05:49 "Nick" <3-nospam(a)temporary-address.org.uk> wrote in message news:878w854hmy.fsf(a)temporary-address.org.uk... > Keith Thompson <kst-u(a)mib.org> writes: >> No, the loops test for ``!a''. ``a'' is a pointer, initialized to 0 >> (NULL). ``data'' is presumably a 3-D array of int. When the item >> is found, a is set to point to the item; even if the item's value >> is 0, a is still non-zero (non-NULL). > > Bloody hell, so it is. So I want to look for an integer, so I create a > pointer to look for it, just so that I can I use null as a distinct > signal value. Er, yes. Without being able to jump out of the loops, there aren't too many alternatives. (Perhaps setting each index to the maximum bound, but in this example the bound expressions were complex.) Anyway, in this case, the return value from the function *was* a pointer to the item. > Of course, "a" was a particularly carefully chosen name to make this > clear. > > And that's the better and clearer code? Yeah, right! "a" is for "address"... The claim was that removing the exit from the inner loop would result in a "MASSIVE" loss of clarity. However the for-loop was written in such a way, even my slightly streamlined version, that a short variable like "a" might well get lost in the noise: for(size_t zInd = 0; zInd < data[xInd][yInd].size(); ++zInd) /* original */ I if I had to write such a loop, in C-style, and in the context of a 10-line function called "findvalue", it might look like: for (z=0; z<zsize; ++z) with zsize set outside the loops. (But then, I wouldn't be bothered myself about using return, break or goto.) -- Bartc
From: Daniel T. on 30 Apr 2010 10:36 tonydee <tony_in_da_uk(a)yahoo.co.uk> wrote: > On Apr 29, 8:51�pm, "Daniel T." <danie...(a)earthlink.net> wrote: > > Nick Keighley <nick_keighley_nos...(a)hotmail.com> wrote: > > > On 28 Apr, 13:59, "Daniel T." <danie...(a)earthlink.net> wrote: > > > > �SG <s.gesem...(a)gmail.com> wrote: > > > > > On 28 Apr., 11:02, Richard Heathfield wrote: > > > > > > > > > > � Value_t* MyClass::findValue(const Value_t& value) > > > > > � { > > > > > � � Value_t* result = 0; > > > > > � � for(size_t xInd = 0; !result && xInd < data.size(); ++xInd) > > > > > � � � for(size_t yInd = 0; !result && yInd < data[xInd].size(); > > > > > � � � � � ++yInd) > > > > > � � � � for(size_t zInd = 0; !result && zInd < data[xInd] > > > > > [yInd].size(); > > > > > � � � � � � ++zInd) > > > > > � � � � { > > > > > � � � � � if(data[xInd][yInd][zInd] == value) > > > > > � � � � � � result = &data[xInd][yInd][zInd]; > > > > > � � � � } > > > > > � � return result; > > > > > � } > > > > > > > > > > I don't know about you but I like the first version better. It's more > > > > > concise. I find it easier to see what the loop's doing. Maybe it's > > > > > just me. I guess I'm used to these kinds of loops. > > > > > > > > Since the sample code is obviously in c++, I would rather see something > > > > like: > > > > > > > > Iterator it = data.begin() > > > > while(it != data.end() && *it != value) > > > > � �++it; > > > > return it != data.end(); > > [snip] > > The point of my example was to show that the problem with Juha's code > > wasn't that it had multiple exits, but rather that it was at the wrong > > level of abstraction and therefore seemed to need multiple exits. > > Richard, removed the multiple exits without fixing the abstraction > > problem and he ended up with a worse result. > > That's a very insightful point, and in many ways good practice - > particularly if it prevents client code coupling to the implementation > or layout of your container. In this case though, the find function > might be a member of the container - inside the encapsulation - and > coupling quite acceptable. Further, say you want something just > marginally more complicated - like skipping every second value of > yInd: you can't do that with your iterator (the x/y/z boundaries are > lost), but it would be a trivial modification to the explicit loop. > Localisation, concision, directness and simplicity must be balanced > against abstraction - the latter is not always better. So, while I > agree an iterator may sometimes be good, I strongly disagree that the > explicitly nested loops were inherently at the wrong level of > abstraction, yielding a necessarily worse result. In this case, the nested for loops were the wrong level, even if the code was embedded in a member function. For your, rather arbitrary, modification they may not be, but even there an iterator that knows how to skip every second value of yInd might be more appropriate IMHO (if such a construct is needed once for this container type, it probably is needed in other places and duplicating the nested for loop construct is even worse than putting it in once. That said, finding examples where multiple returns are a good idea is not hard, even with the code I presented something like this: for (Iterator it = data.begin(); it != data.end(); ++it) if (*it == value) return true; return false; Here I have removed the extra conditional by introducing an extra return. It's not how I would write it, but the above is quite acceptable IMHO. There is a general pattern here I think. When there are multiple exit conditions to a loop (there is an && in the exit condition,) and we have to be able to distinguish after the loop which condition caused it to exit, then it may be appropriate to have multiple exits from the loop. I'm just musing here, but what do you guys think?
From: Nick on 30 Apr 2010 14:41
"Daniel T." <daniel_t(a)earthlink.net> writes: > There is a general pattern here I think. When there are multiple exit > conditions to a loop (there is an && in the exit condition,) and we have > to be able to distinguish after the loop which condition caused it to > exit, then it may be appropriate to have multiple exits from the loop. > I'm just musing here, but what do you guys think? I'm very happy with it. My view is that leaving early is perfectly all right in at least two circumstances: a) in a "do I do this at all?" check at the start of some code b) when you've finished what you came to do (like finding something in a search function). For (a), I'd much rather see enum retval do_something(char *s) { if(s == NULL) return no_string; if(!*s) return empty_string; if(strlen(s) < 3) return short_string; while(*s) { if(....) } return not_found; } than something involving a pile of nested ifs and a variable to hold the eventual return value. This way you *know* that all's over with, without reading down to the bottom to check. -- Online waterways route planner | http://canalplan.eu Plan trips, see photos, check facilities | http://canalplan.org.uk |