on goto
in [General Programming]
|
From: Dave Harris on 10 May 2010 09:04 daniel_t(a)earthlink.net (Daniel T.) wrote (abridged): > > Yes. It led to simple code, but that was partly because all the > > complexity was hidden behind an iterator. > > [...] > I didn't post anymore than I did because there isn't enough context > to tell what the best solution is. Even though .size() is checked > at each level, there may still be an invariant such that all the > sizes are the same. I would be interested in code that solved the same problem that the original code solved. Or are you agreeing that the original code was the best, for that problem? Can you only do better if you change the problem by adding extra invariants? -- Dave Harris, Nottingham, UK.
From: Nathan on 10 May 2010 10:28 On May 10, 9:04 am, brang...(a)cix.co.uk (Dave Harris) wrote: > nathancba...(a)gmail.com (Nathan Baker) wrote (abridged): > > > > Yes. It led to simple code, but that was partly because all the > > > complexity was hidden behind an iterator. Nathan Baker's version > > > exposes some of that complexity, and I think illustrates how hard > > > it is to get it right. > > > Perhaps the words "simple" and "complexity" have different meanings > > to different people because of their different training and the goals > > they've been conditioned to target?? > > > Juha's code uses 3 loop constructs, 4 conditionals, and 2 exit > > points. > > > Mine uses 1 loop construct, 2 conditionals, and 1 exit point. > > But your code was broken in the ways pointed out by other posters. In > particular, it accesses data[0][0][0] even if data[0].size() is zero, > which will give a bounds error or a crash. It also failed to return a the > address where the item was found, and was incapable of searching for > value 0. It also did huge amounts of unnecessary work, in that the inner > loop tested all of the conditionals. (And by my count it uses a lot more > than 2 conditionals even as posted.) You need to fix all the bugs in your > code before we can even consider its complexity. > > When I try to write code myself that uses your approach, but correctly > handles all the cases the original code handles, I get a horrible mess. > Specifically, you need to be aware that data[0][0].size() may be > different to data[1][0].size() or data[0][1].size(), and that any of them > may be zero meaning that (eg) data[0][0][0] does not exist and must not > be accessed. Similarly for data[0].size() and data.size(), and data[0][0] > and data[0]. > > This seems to mean you need a loop to find the first accessible value > before you even look at data[xInd][yInd][zInd]. > Then let's just start with Juha's original code and see where some simple improvements can be made, shall we? Given, the original... ,--- Value_t* MyClass::findValue(const Value_t& value) { for(size_t xInd = 0; xInd < data.size(); ++xInd) for(size_t yInd = 0; yInd < data[xInd].size(); ++yInd) for(size_t zInd = 0; zInd < data[xInd][yInd].size(); + +zInd) { if(data[xInd][yInd][zInd] == value) return &data[xInd][yInd][zInd]; } return 0; } `--- First, we can remove the nested loops like so... ,--- Value_t* MyClass::findValue(const Value_t& value) { size_t xyzMax = data.size() + data[xInd].size() + data[xInd] [yInd].size(); for(size_t xInd = 0; xInd < xyzMax; ++xInd) { if(data[xInd] == value) return &data[xInd]; } return 0; } `--- Next, we remove the nested exit point by changing it into a signal to exit the loop... ,--- Value_t* MyClass::findValue(const Value_t& value) { size_t xyzMax = data.size() + data[xInd].size() + data[xInd] [yInd].size(); result = 0; for(size_t xInd = 0; xInd < xyzMax; ++xInd) { if(data[xInd] == value) result = &data[xInd]; xInd = xyzMax; } return result; } `--- Juha asked for an alternative that didn't make use of multiple exit points. This is one way to achieve that -- fold the inner loops (those indices are meaningless) and signal the end to the 'for' loop. So, 3 loop constructs are reduced to 1 4 conditionals are reduced to 2 2 exit points are reduced to 1 Simple, isn't it? Nathan.
From: Ben Bacarisse on 10 May 2010 11:06 [I've trimmed the groups. Sorry if this is the wrong thing to do] Nathan <nathancbaker(a)gmail.com> writes: <snip> > Then let's just start with Juha's original code and see where some > simple improvements can be made, shall we? > > Given, the original... > > ,--- > Value_t* MyClass::findValue(const Value_t& value) > { > for(size_t xInd = 0; xInd < data.size(); ++xInd) > for(size_t yInd = 0; yInd < data[xInd].size(); ++yInd) > for(size_t zInd = 0; zInd < data[xInd][yInd].size(); ++zInd) > { > if(data[xInd][yInd][zInd] == value) > return &data[xInd][yInd][zInd]; > } > > return 0; > } > `--- > > First, we can remove the nested loops like so... > > ,--- > Value_t* MyClass::findValue(const Value_t& value) > { > size_t xyzMax = data.size() + data[xInd].size() + data[xInd] > [yInd].size(); Did you mean to multiply here? > for(size_t xInd = 0; xInd < xyzMax; ++xInd) > { > if(data[xInd] == value) You can't assume that the data is contiguous or, to be a little more general, that a single [] operation means the same as three of them. This is, after all, C++ not C. > return &data[xInd]; > } > > return 0; > } > `--- > > Next, we remove the nested exit point by changing it into a signal to > exit the loop... > > ,--- > Value_t* MyClass::findValue(const Value_t& value) > { > size_t xyzMax = data.size() + data[xInd].size() + data[xInd] > [yInd].size(); > result = 0; > for(size_t xInd = 0; xInd < xyzMax; ++xInd) > { > if(data[xInd] == value) > result = &data[xInd]; > xInd = xyzMax; > } > > return result; > } > `--- > > Juha asked for an alternative that didn't make use of multiple exit > points. This is one way to achieve that -- fold the inner loops > (those indices are meaningless) and signal the end to the 'for' loop. > > So, > > 3 loop constructs are reduced to 1 > 4 conditionals are reduced to 2 These two are the same, in effect. It distorts the picture if you count the reduction in loops and then the inevitable consequences of that reduction. > 2 exit points are reduced to 1 And you've added the potential for confusion -- maybe even future bugs. E.g. what, if anything, can we say about xInd after the loop? Rather than signal the end by setting xInd, what objection could you have to a 'break' statement there? How is setting xInd better? > Simple, isn't it? I don't think it's correct yet so its simplicity is not really the main issue. -- Ben.
From: Thomas J. Gritzan on 10 May 2010 11:16 Am 10.05.2010 16:28, schrieb Nathan: > On May 10, 9:04 am, brang...(a)cix.co.uk (Dave Harris) wrote: >> This seems to mean you need a loop to find the first accessible value >> before you even look at data[xInd][yInd][zInd]. >> > > Then let's just start with Juha's original code and see where some > simple improvements can be made, shall we? > > Given, the original... > > ,--- > Value_t* MyClass::findValue(const Value_t& value) > { > for(size_t xInd = 0; xInd < data.size(); ++xInd) > for(size_t yInd = 0; yInd < data[xInd].size(); ++yInd) > for(size_t zInd = 0; zInd < data[xInd][yInd].size(); + > +zInd) > { > if(data[xInd][yInd][zInd] == value) > return &data[xInd][yInd][zInd]; > } > > return 0; > } > `--- This code iterates over a 3D matrix, so it uses 3 indices. > First, we can remove the nested loops like so... > > ,--- > Value_t* MyClass::findValue(const Value_t& value) > { > size_t xyzMax = data.size() + data[xInd].size() + data[xInd] > [yInd].size(); > for(size_t xInd = 0; xInd < xyzMax; ++xInd) > { > if(data[xInd] == value) > return &data[xInd]; > } > > return 0; > } > `--- This code uses only 1 index operation to data. data[xInd] still has 2 dimensions and can't be compared to the scalar 'value'; the types don't match. Also, you don't go over x_size*y_size*z_size elements but only x_size+y_size+z_size elements. The code doesn't do the same as the other one. [...] > Juha asked for an alternative that didn't make use of multiple exit > points. This is one way to achieve that -- fold the inner loops > (those indices are meaningless) and signal the end to the 'for' loop. > > So, > > 3 loop constructs are reduced to 1 > 4 conditionals are reduced to 2 > 2 exit points are reduced to 1 > > Simple, isn't it? We can also remove all the loops and the function would even be more efficient: Value_t* MyClass::findValue(const Value_t& value) { return 0; // Single Exit point! } Much more simple, isn't it? -- Thomas
From: Willem on 10 May 2010 11:51
Richard Heathfield wrote: ) Seebs wrote: )> On 2010-05-10, Richard Heathfield <rjh(a)see.sig.invalid> wrote: )>> Yes. But, for me, SESE is idiomatic. )> )> That's not an idiom, that's a policy. ) ) Yes, but it's an idiomatic policy. A high-level idiom, if you like. I don't like. The word 'idiom' speaks of 'an expression, word or phrase'. So no, SESE cannot possibly be idiomatic by any measure of the word. ) I understand the point behind your reply, of course. It's just that I ) don't agree with it. (As with many style issues, there is no one right ) answer that works for everybody.) I'm not seeing any refutations here of the arguments that Seebs presented. Perhaps you're not understanding the actual point, but instead thinking of a different point that you can dismiss easily. This happens a lot. SaSW, Willem -- Disclaimer: I am in no way responsible for any of the statements made in the above text. For all I know I might be drugged or something.. No I'm not paranoid. You all think I'm paranoid, don't you ! #EOT |