Would this pass code review where you are?

Fueled By Decaff

First thoughts: It does not look very performant with 4 nested for loops, but it might be necessary, so need more information before commenting any more. Blink twice thoughts: Nuke that goto. Nuke it from space... This should not compile in C#. Wouldn't that have undefined behaviour in C, as the loop variables would not get initialised? It looks like the code that the goto label refers to is only executed from the goto, so perhaps the goto could be replaced by moving that code into the if statement. Good luck untangling this code.

DerekT P

Well, it shows a good deal of promise. The author has very rigorously and helpfully identified (via comments) both the end of the method, and the end of the last for loop. Clearly s/he was in a great hurry, or maybe just wilfully lazy, as there is no equivalent commenting on the end of the inner loops. A smack on the wrist needed there, I think. Succinct and meaningful variable names used. i_n_ suggests a background in early BASIC, but that's fine; at least we know which level each variable relates to. There's a flag helpfully named flag, and a condition sensibly named pass. Well thought through. My criticism would be that ep is obviously the "end procedure" goto target, but to perpetuate the camel case standard used in the method name, it should be eP. Indentation is good and consistent. Not sure on the inline if but that may conform to local standards. Otherwise? meh. :|

Jacquers

Without context on what it's doing and trying to solve it's a bit difficult to say, but it looks klunky and if I had to review it I'd probably fail it.

Fueled By Decaff

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

Pete OHanlon

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.

Advanced TypeScript Programming Projects

theoldfool

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.

realJSOP

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

PIEBALDconsult

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:

Jin Vincent Necesario

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/.

Mike Hankey

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

OriginalGriff

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!

raddevus

5teveH wrote:

You need to tell the author he's in the wrong job.

:thumbsup::thumbsup::thumbsup: :laugh:

raddevus

Sander Rossel wrote:

For-Oriented Programming, or FOP

:laugh:

MarkTJohnson

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.

Gary R Wheeler

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 the ep symbol is intended to be a label, it's certainly poorly placed. 3. There's nothing inherently wrong in the numerous nested for loops. 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 values i1, i2, etc. are poorly named. 4. I also don't like the return embedded in the loops. I don't have a problem using break or continue to 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;

Lost User

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

Rick York

We don't have code reviews but I would have one with the group just to point out how horrendous that is.

"They have a consciousness, they have a life, they have a soul! Damn you! Let the rabbits wear glasses! Save our brothers! Can I get an amen?"

Lost User

Commenting the ending brace shows potential.

It was only in wine that he laid down no limit for himself, but he did not allow himself to be confused by it. ― Confucian Analects: Rules of Confucius about his food

jsc42

raddevus wrote:

Sander Rossel wrote:

For-Oriented Programming, or FOP

Or For Loop Oriented Programming - guaranteed to be a complete FLOP (Yes, I know FLOPS is already an acronym in computing for Floating Point Operations Per Second).

John Torjo

He would probably ace politics...