Assignment in method call bad practice?

Question

This is my first question to Stackoverflow, although I have been a consumer for many years. Please forgive me if I break rules. That is certainly not my intent. I have strenuously reviewed the rules and believe I am within the bounds of acceptability. However, I would ask that you kindly point out usage errors, if they are present, so that I may in future be more compliant.

I teach programming to high school students. This semester we are doing C#. Last week we were studying recursion. I assigned several classic problems that can be solved with recursion, one of them being exponentiation.

One of my students submitted the following code as a solution for exponentiation using recursion (he did give me permission to post it here). The assignment in the method call gave me the willies, but when I told him that it was bad practice, he protested, saying that "it works", that it "made sense" to him, and that he "does it all the time".

static void Recur(int n1, int n2, int n3) 
{ 
   if (n2 > 0) 
   { 
      Recur(n1, n2 - 1, n3 *= n1);   // this is the line in question
   } 
   else 
   { 
      Console.WriteLine("The number calculated recursively is: {0}", n3); 
   } 
} 

I have had a hard time coming up with something concrete to tell my student about why making an assignment in a method call is bad practice in general, other than 1) the possibility of unintended side effects, and 2) the difficulty of maintenance.

I have searched online with every phrase I can construct concerning this question, but have come up pretty empty-handed. I did see a reference to a book called "Clean Code" by Robert C. Martin that I don't own.

My relationship with my students is somewhat like that of a parent. They sometimes don't put a lot of stock in what I say unless I can corroborate it with an independent source. If I could point to a definitive statement about putting assignments in a method call my student would be more inclined to stop this annoying habit.

Does this usage bother anyone else? Am I expecting too much in wanting him to change the way he's been doing things? He's 15, but with a promising future ahead of him. He's one of those students who just "gets it". I don't want him to develop bad practices.

Thank you for your time and input.

Chyme

In that context it is pointless because the value of n3 is not used after it has been changed - and pointless code is noise, and noise is bad. And even if it was used, then where it is changed is buried inside a method call making it hard to spot, which is also bad. (Note that code which does nothing but which makes the code easier to understand is NOT pointless and is OK.) – Intrados

This should be asked on CodeReview.SE because it is about style and not broken code. – Lumbard

I'd say that having an assignment in the method call isn't necessarily bad as long as it is clear what is going on. Recursion however should be avoided like the plague. It is a good skill to understand, but mostly for the purpose of avoiding it. If you are teaching recursion, I'd say to make the next lesson about doing the same thing without recursion and discuss why you would choose one over the other. – Lumbard

To add to my previous comment: Tell your student that Recur(n1, n2 - 1, n3 * n1); will also work, so the = is unnecessary. – Intrados

Rather than looking for a citable source to point to, why not help him prove why it's a bad design? E.g., do a test that forces an overflow, and show him where and how the debugger complains with his code vs your preferred code with "standard" recursion (tail or normal). Then alter the spec (this is software, clients always alter spec) in some way, and ask for a refactoring. Which one is easier to refactor? Lastly, I think it's important that he understand why he's chosen this manner of recursion than a more familiar one: if you can't defend a code choice, then do you really understand it? – Lulualaba

OK, Ron Beyer, thanks for the pointer to CodeReview.SE. – Chyme

We are studying recursion for the purpose of honing logic skills, and also to start introducing the idea of stack vs. heap. Every problem we have solved recursively we have also solved iteratively. Recursion may need to be avoided, but it should still be understood. I personally like to solve problems recursively just because I think it's fun. I also realize that in the "real world" it is commonly avoided. But as you know it is part of several solutions to sorting and searching, which we will be studying shortly. – Chyme

Thank you K_foxer9 for the idea to get him to prove my point himself. I will work on that. – Chyme

You might also be interested in cseducators.stackexchange.com – Protocol

Thanks pinkfloydx33 I just discovered that! – Chyme

This may also be a good time to teach a lesson in separation of concerns - the example is mixing business logic and I/O. Change the return type to int and replace the Console.WriteLine(...); with return n3; Let the calling method worry about displaying the final calculated value. – Corium

Answer 1

There are quite a few things improvable in the posted code:

1. Be consistent

   Why isn't the call Recur(n1, n2 =- 1, n3 *= n1)? Why is n3 treated differently? This can cause confusion when reviewing/mantaining code.

2. Dont do unnecessary or superfluous work

   Is n3 ever used after Recur(n1, n2 - 1, n3 *= n1)? No? Then why waste time assigning a value that is never used?

3. It's considered good practice to not mutate method arguments (unless they are passed in by reference of course). Why? Because it makes debugging and understanding how the code works much harder; having the initial conditions mutate as the method executes makes it much harder to track possible bugs, optimizations, improvements, etc.

All that said, I avoid these kind of patterns because I have really bad memory:

var i = 0;
Foo(i += 1);
Bar(i);

What is passed into Foo? Was it 0 or was it 1? I never remember and I have to look it up everytime. Chances are that whoever reviews this code can have the same problem. These clever tricks don't make the code work any faster or better and it avoids a grand total of one code line... not worth it.

General

Answer 2

The bad code is this one: n3 *= n1 Your student doesn't see any problem because method parameters are not final. Just define them final and you will see this peace of code will cause a compile error.

Why should one use final? Because this makes sure the method parameters will not be used for any other purposes inside the method. This makes the code less error prone, more reliable. Using one variably for one purpose makes also the code easier to understand by others which is important in the industry.

And as already pointed by others, assigning a value to n3 makes not sense because further on variable n3 is not used in the code.

His statement that this "makes sense to him" shows that he does not about software development in the real life, in the industry - starting from small companies and to Google or Facebook. One of the requirements is that the code should be maintainable. This peace of code is not.

Regarding "with a promising future ahead of him": How do you know that? Did he win any programming competition? His argument that he "does it all the time" is pretty weak. Ask him why does he do it? Give him a small research about the code quality. May be this will help him to understand software development a little bit.

Peplos