Would this pass code review where you are?
-
I didn't write it, but ... the author thinks this is a good idea that should be allowed in C# code:
getSeries()
{if (flag == pass) goto ep;
for i1 ...
...
for i2 ...
...
for i3 ...
...
for i4 ...
.
.
.
for ...
{
...
return;
ep;
}
}
}
}//last for}// end of getSeries()
Me? I'm not a fan. Your thoughts - and remember this is the Lounge? :laugh:
"I have no idea what I did, but I'm taking full credit for it." - ThisOldTony "Common sense is so rare these days, it should be classified as a super power" - Random T-shirt AntiTwitter: @DalekDave is now a follower!
-
Apparently so, in C++. In C# (his target language) no - the label is out of scope of the goto. It was apparently written in 2006 ... and in my opinion you shouldn't try to perpetuate code crimes into new languages. To be honest, any coder who thinks nesting loops 5+ deep is a good idea probably needs a lesson in exponential growth ... :laugh:
"I have no idea what I did, but I'm taking full credit for it." - ThisOldTony "Common sense is so rare these days, it should be classified as a super power" - Random T-shirt AntiTwitter: @DalekDave is now a follower!
Ooh, I thought C++ initialised variables to 0 by default - apparently not. So this is undefined behaviour if any of the loop variables are used after the goto is executed. Fnu
-
I didn't write it, but ... the author thinks this is a good idea that should be allowed in C# code:
getSeries()
{if (flag == pass) goto ep;
for i1 ...
...
for i2 ...
...
for i3 ...
...
for i4 ...
.
.
.
for ...
{
...
return;
ep;
}
}
}
}//last for}// end of getSeries()
Me? I'm not a fan. Your thoughts - and remember this is the Lounge? :laugh:
"I have no idea what I did, but I'm taking full credit for it." - ThisOldTony "Common sense is so rare these days, it should be classified as a super power" - Random T-shirt AntiTwitter: @DalekDave is now a follower!
Without context, it's hard to say what's going on but a thought occurs to me here; if there is no condition before that return statement, each loop level is going to execute only once so the whole logic could be flattened.
-
Without context, it's hard to say what's going on but a thought occurs to me here; if there is no condition before that return statement, each loop level is going to execute only once so the whole logic could be flattened.
who gave you permission to post my code on here? I thought the red printing at the top forbid that. edit: oops, sorry meant to reply to that other fellow.
If you can keep your head while those about you are losing theirs, perhaps you don't understand the situation.
-
Not sure what it does, especially goto part. But reminds of person who skipped recursion class...
Without knowing the range of the for loops, and how many there are, I wouldn't recommend recursion, because it might overflow the stack.
".45 ACP - because shooting twice is just silly" - JSOP, 2010
-----
You can never have too much ammo - unless you're swimming, or on fire. - JSOP, 2010
-----
When you pry the gun from my cold dead hands, be careful - the barrel will be very hot. - JSOP, 2013 -
I didn't write it, but ... the author thinks this is a good idea that should be allowed in C# code:
getSeries()
{if (flag == pass) goto ep;
for i1 ...
...
for i2 ...
...
for i3 ...
...
for i4 ...
.
.
.
for ...
{
...
return;
ep;
}
}
}
}//last for}// end of getSeries()
Me? I'm not a fan. Your thoughts - and remember this is the Lounge? :laugh:
"I have no idea what I did, but I'm taking full credit for it." - ThisOldTony "Common sense is so rare these days, it should be classified as a super power" - Random T-shirt AntiTwitter: @DalekDave is now a follower!
OriginalGriff wrote:
code review where you are
On my current team, I'm the only one who writes "real code" -- and I definitely would not have written that. We've been required to do code reviews though... so we pretend to do code reviews of our SSIS packages. :confused:
-
I didn't write it, but ... the author thinks this is a good idea that should be allowed in C# code:
getSeries()
{if (flag == pass) goto ep;
for i1 ...
...
for i2 ...
...
for i3 ...
...
for i4 ...
.
.
.
for ...
{
...
return;
ep;
}
}
}
}//last for}// end of getSeries()
Me? I'm not a fan. Your thoughts - and remember this is the Lounge? :laugh:
"I have no idea what I did, but I'm taking full credit for it." - ThisOldTony "Common sense is so rare these days, it should be classified as a super power" - Random T-shirt AntiTwitter: @DalekDave is now a follower!
Maybe a self code review or the code review was sent to a subordinate/junior guy or a force approval. :)
Jim Rohn: "Don't wish it was easier, wish you were better". Subscribe to my blog @ https://jinnecesario.com/.
-
I didn't write it, but ... the author thinks this is a good idea that should be allowed in C# code:
getSeries()
{if (flag == pass) goto ep;
for i1 ...
...
for i2 ...
...
for i3 ...
...
for i4 ...
.
.
.
for ...
{
...
return;
ep;
}
}
}
}//last for}// end of getSeries()
Me? I'm not a fan. Your thoughts - and remember this is the Lounge? :laugh:
"I have no idea what I did, but I'm taking full credit for it." - ThisOldTony "Common sense is so rare these days, it should be classified as a super power" - Random T-shirt AntiTwitter: @DalekDave is now a follower!
Isn't that the guy in QA that asked where to stick the goto? But I'm sure that's not where they told him to stick it.
I'm not sure how many cookies it makes to be happy, but so far it's not 27. JaxCoder.com
-
Isn't that the guy in QA that asked where to stick the goto? But I'm sure that's not where they told him to stick it.
I'm not sure how many cookies it makes to be happy, but so far it's not 27. JaxCoder.com
Yes - I'm trying to get him to see that I'm not the only one who looks at theat and sees Assembler code written in C compiled by a C++ compiler and never going to work in C# ... He thinks it's perfectly good code because it has worked since 2006. We all know that just because you can do something, it doesn't mean you should. I feel sorry for anyone who has to work for / with him - that company has to be the kiss of death for your career!
"I have no idea what I did, but I'm taking full credit for it." - ThisOldTony "Common sense is so rare these days, it should be classified as a super power" - Random T-shirt AntiTwitter: @DalekDave is now a follower!
-
You need to tell the author he's in the wrong job. He needs to find something where logic and common sense isn't required. Management, maybe? ;)
-
It's this new cool style of programming named For-Oriented Programming, or FOP for short. I think I'll try it out on my next project :thumbsup:
Best, Sander Azure Serverless Succinctly Migrating Applications to the Cloud with Azure arrgh.js - Bringing LINQ to JavaScript
-
Yes - I'm trying to get him to see that I'm not the only one who looks at theat and sees Assembler code written in C compiled by a C++ compiler and never going to work in C# ... He thinks it's perfectly good code because it has worked since 2006. We all know that just because you can do something, it doesn't mean you should. I feel sorry for anyone who has to work for / with him - that company has to be the kiss of death for your career!
"I have no idea what I did, but I'm taking full credit for it." - ThisOldTony "Common sense is so rare these days, it should be classified as a super power" - Random T-shirt AntiTwitter: @DalekDave is now a follower!
OriginalGriff wrote:
We all know that just because you can do something, it doesn't mean you should.
Make him watch Jurrasic Park. Again. Like Malcom McDowell in Clockwork Orange.
I’ve given up trying to be calm. However, I am open to feeling slightly less agitated.
-
I didn't write it, but ... the author thinks this is a good idea that should be allowed in C# code:
getSeries()
{if (flag == pass) goto ep;
for i1 ...
...
for i2 ...
...
for i3 ...
...
for i4 ...
.
.
.
for ...
{
...
return;
ep;
}
}
}
}//last for}// end of getSeries()
Me? I'm not a fan. Your thoughts - and remember this is the Lounge? :laugh:
"I have no idea what I did, but I'm taking full credit for it." - ThisOldTony "Common sense is so rare these days, it should be classified as a super power" - Random T-shirt AntiTwitter: @DalekDave is now a follower!
We don't do code reviews where I work. Go ahead, keep throwing up; I'll wait. 1. I don't like the
goto. I know some folks like them for error exit handling when you're not using exceptions, but I'm not one of them. 2. If theepsymbol is intended to be a label, it's certainly poorly placed. 3. There's nothing inherently wrong in the numerous nestedforloops. It depends upon their purpose. 3.1. If the nested loops are simple indices in multi-dimensional data performing a simple task, it might be the most concise way to accomplish that task. Adding layers of abstraction to remove the nesting might complicate the logic unnecessarily, especially if that's the only reason for adding the layer. 3.2. If 3.1 is not the case, the loops are certainly a code smell since they imply accessing multiple levels of detail from a single scope. 3.3. The iteration valuesi1,i2, etc. are poorly named. 4. I also don't like thereturnembedded in the loops. I don't have a problem usingbreakorcontinueto exit a loop early, but that keeps the range of the'goto'to the body of the loop, making control flow analysis simpler.Software Zen:
delete this; -
I didn't write it, but ... the author thinks this is a good idea that should be allowed in C# code:
getSeries()
{if (flag == pass) goto ep;
for i1 ...
...
for i2 ...
...
for i3 ...
...
for i4 ...
.
.
.
for ...
{
...
return;
ep;
}
}
}
}//last for}// end of getSeries()
Me? I'm not a fan. Your thoughts - and remember this is the Lounge? :laugh:
"I have no idea what I did, but I'm taking full credit for it." - ThisOldTony "Common sense is so rare these days, it should be classified as a super power" - Random T-shirt AntiTwitter: @DalekDave is now a follower!
OriginalGriff wrote:
Your thoughts
The label 'ep' probably means exit point. The only thing I can think of is that the author is following the single-entry single-exit[^] code standard. I've worked on teams that tried to adhere to this policy. I've had some of my code rejected by reviewers with the reason of: "Refactor to single exit". That code is in your operating system. Best Wishes, -David Delaune
-
I didn't write it, but ... the author thinks this is a good idea that should be allowed in C# code:
getSeries()
{if (flag == pass) goto ep;
for i1 ...
...
for i2 ...
...
for i3 ...
...
for i4 ...
.
.
.
for ...
{
...
return;
ep;
}
}
}
}//last for}// end of getSeries()
Me? I'm not a fan. Your thoughts - and remember this is the Lounge? :laugh:
"I have no idea what I did, but I'm taking full credit for it." - ThisOldTony "Common sense is so rare these days, it should be classified as a super power" - Random T-shirt AntiTwitter: @DalekDave is now a follower!
-
I didn't write it, but ... the author thinks this is a good idea that should be allowed in C# code:
getSeries()
{if (flag == pass) goto ep;
for i1 ...
...
for i2 ...
...
for i3 ...
...
for i4 ...
.
.
.
for ...
{
...
return;
ep;
}
}
}
}//last for}// end of getSeries()
Me? I'm not a fan. Your thoughts - and remember this is the Lounge? :laugh:
"I have no idea what I did, but I'm taking full credit for it." - ThisOldTony "Common sense is so rare these days, it should be classified as a super power" - Random T-shirt AntiTwitter: @DalekDave is now a follower!
-
You need to tell the author he's in the wrong job. He needs to find something where logic and common sense isn't required. Management, maybe? ;)
He would probably ace politics...
-
I didn't write it, but ... the author thinks this is a good idea that should be allowed in C# code:
getSeries()
{if (flag == pass) goto ep;
for i1 ...
...
for i2 ...
...
for i3 ...
...
for i4 ...
.
.
.
for ...
{
...
return;
ep;
}
}
}
}//last for}// end of getSeries()
Me? I'm not a fan. Your thoughts - and remember this is the Lounge? :laugh:
"I have no idea what I did, but I'm taking full credit for it." - ThisOldTony "Common sense is so rare these days, it should be classified as a super power" - Random T-shirt AntiTwitter: @DalekDave is now a follower!
No, simply for nesting / length. I'm sure well-named functions could be pulled out, which would decrease the cognitive load on the reader, especially someone new to the code, which includes the original author in about a month or two.
------------------------------------------------ If you say that getting the money is the most important thing You will spend your life completely wasting your time You will be doing things you don't like doing In order to go on living That is, to go on doing things you don't like doing Which is stupid. - Alan Watts https://www.youtube.com/watch?v=-gXTZM\_uPMY
-
I didn't write it, but ... the author thinks this is a good idea that should be allowed in C# code:
getSeries()
{if (flag == pass) goto ep;
for i1 ...
...
for i2 ...
...
for i3 ...
...
for i4 ...
.
.
.
for ...
{
...
return;
ep;
}
}
}
}//last for}// end of getSeries()
Me? I'm not a fan. Your thoughts - and remember this is the Lounge? :laugh:
"I have no idea what I did, but I'm taking full credit for it." - ThisOldTony "Common sense is so rare these days, it should be classified as a super power" - Random T-shirt AntiTwitter: @DalekDave is now a follower!
Pascal, for instance, will not allow you to hyperspace into a scope like that. for good reason. the only useful scenario for this "technique" is where you have the expressions after the closing brace of the inner loops. that way either you execute all the looping which creates some multi dimensional computation or you jump with goto inside the most inner loop and execute a liner computation, once for every loop. but, in this case i fail to comprehend the reason for jumping at the tail of the loops. equally puzzled by the most inner return. this looks like is some kind of a trick interview question... it would be interesting to know what is the purpose of this "device"?
