1 Function or multiple

[Guest]

Many programmers say that you should have a function for every thing that has to be done & that each function should only have 1 purpose. I was writing a Monty Hall simulator in C# with WPF and just reviewed the code a bit. 

For the instances of functions

"acceptButton_Click" 

Spoiler

private void acceptButton_Click(object sender, EventArgs e)
        {

            bool GuessedCorrect = false;
#region radio Button Check
            if (doorButton1.Checked == true) {
                GuessedCorrect = CorrectChoice(1);
            }
            else if (doorButton2.Checked == true) {
                GuessedCorrect = CorrectChoice(2);
            }
            else if (doorButton3.Checked == true) {
                GuessedCorrect = CorrectChoice(3);
            }
#endregion

            //Runs if the player guessed the right door and tells the player he wins. 
            if (GuessedCorrect == true) {
                informationText.Text = "You guessed correctly. You win! :D";
                acceptButton.Enabled = false;
                return;
                
            }

            //Runs if this is the player's second guess and didn't get the answer correct. Ends the game. 
            if (guessedOnce == true) {
                int doorThatHasCar = -1; //Debug value assigned. If number shows up -1, there was an error in assigning true to a door or the following loop didn't find the door with the car. 
                for(int i = 0; i < doorHasCar.Length; i++) { //Finds the door with the car behind it. 
                    if (doorHasCar[i] == true) {
                        doorThatHasCar = (i + 1); //Assigns variable the real number (not the index) of the door that has the car. 
                    }
                }
                informationText.Text = "Sorry. The car was behind door " + doorThatHasCar + ". Better luck next time! Click restart to try again.";
                acceptButton.Enabled = false;
                return;
            }

            //Runs if this is player's first guess & didn't get answer correct. 
            informationText.Text = "Not Correct this time. I'll give you one last chance to get it right.";
            guessedOnce = true;
        }

 

"NewGame"

Spoiler

private void NewGame() {
            informationText.Text = "Welcome to the Monty Hall Simulator. " +
                "There is a new car behind one of the 3 doors." +
                "Guess which door it is & win yourself a new car!";

            //Allows player to guess 2 times. 
            guessedOnce = false;

            //Allows player to guess
            acceptButton.Enabled = true;

            //Resets all doors to not have car
            for (int i = 0; i < doorHasCar.Length; i++) {
                 doorHasCar[i] = false;
            }

            //Sets 1 door to have car behind it.
            Random rnd = new Random();
            int highestIndexInArray = doorHasCar.Length - 1;
            doorHasCar[rnd.Next(0, highestIndexInArray)] = true;
        }

 

 

They do several things. New game handles all the resetting & accept button handles all the win/lose conditions. 

 

As this is a small program & it really depends on the programmer's thoughts, what do you think would be best? 
In similar situations or when you're positive logic won't be used anywhere else in the program, do you think separate functions should be made? 

It's good practice to always make everything separate so I've heard but in the other hand if it's only ever going to be called from one place, should it not be put into a separate function? 

Spoiler

Full class if you're interested:

using System;
using System.Collections.Generic;
using System.ComponentModel;
using System.Data;
using System.Drawing;
using System.Linq;
using System.Text;
using System.Windows.Forms;

namespace MontyHall
{
    public partial class Form1 : Form
    {

        bool[] doorHasCar = new bool[3];
        bool guessedOnce;

        public Form1()
        {
            InitializeComponent();
            NewGame();
        }

        private void NewGame() {
            informationText.Text = "Welcome to the Monty Hall Simulator. " +
                "There is a new car behind one of the 3 doors." +
                "Guess which door it is & win yourself a new car!";

            //Allows player to guess 2 times. 
            guessedOnce = false;

            //Allows player to guess
            acceptButton.Enabled = true;

            //Resets all doors to not have car
            for (int i = 0; i < doorHasCar.Length; i++) {
                 doorHasCar[i] = false;
            }

            //Sets 1 door to have car behind it.
            Random rnd = new Random();
            int highestIndexInArray = doorHasCar.Length - 1;
            doorHasCar[rnd.Next(0, highestIndexInArray)] = true;
        }

        //Checks if player guessed the correct door. 
        private bool CorrectChoice(int choice) {
            choice -= 1;
            if (doorHasCar[choice] == true) {
                return true;
            }
            return false;
        }



        //Unused function. Do NOT DELETE. 
        private void informationText_TextChanged(object sender, EventArgs e)
        {

        }

        //Restarts game
        private void RestartButton_Click(object sender, EventArgs e)
        {
            NewGame();
        }

        private void acceptButton_Click(object sender, EventArgs e)
        {

            bool GuessedCorrect = false;
#region radio Button Check
            if (doorButton1.Checked == true) {
                GuessedCorrect = CorrectChoice(1);
            }
            else if (doorButton2.Checked == true) {
                GuessedCorrect = CorrectChoice(2);
            }
            else if (doorButton3.Checked == true) {
                GuessedCorrect = CorrectChoice(3);
            }
#endregion

            //Runs if the player guessed the right door and tells the player he wins. 
            if (GuessedCorrect == true) {
                informationText.Text = "You guessed correctly. You win! :D";
                acceptButton.Enabled = false;
                return;
                
            }

            //Runs if this is the player's second guess and didn't get the answer correct. Ends the game. 
            if (guessedOnce == true) {
                int doorThatHasCar = -1; //Debug value assigned. If number shows up -1, there was an error in assigning true to a door or the following loop didn't find the door with the car. 
                for(int i = 0; i < doorHasCar.Length; i++) { //Finds the door with the car behind it. 
                    if (doorHasCar[i] == true) {
                        doorThatHasCar = (i + 1); //Assigns variable the real number (not the index) of the door that has the car. 
                    }
                }
                informationText.Text = "Sorry. The car was behind door " + doorThatHasCar + ". Better luck next time! Click restart to try again.";
                acceptButton.Enabled = false;
                return;
            }

            //Runs if this is player's first guess & didn't get answer correct. 
            informationText.Text = "Not Correct this time. I'll give you one last chance to get it right.";
            guessedOnce = true;
        }
    }
}

 

 

[Mira Yurizaki]

It's really a preference thing. If you really want to write 10,000 line functions, that's on you, but few others would agree with that style. And there was a general rule of thumb that if your function can't fit on a page, it's too big. Except what's a page? A screen of text? That's not well defined since even console terminals (which was were the rule of thumb that 80 characters should be the most one line should be) can have a different number of lines (80x20? 80x40?)

 

Okay, that was sidetracking too much. For me, I start thinking about breaking the function up when:

• it's approaching 100 lines or so
• there's needs more than three levels of a conditional or loop (or at that point, I need to rethink the logic)
• I'm copying and pasting something

 

If a function is doing multiple things but fits on say a dozen lines of code, that's fine. Like for example, doing some basic preprocessing of text and writing it to a file.

 

[reniat]

One approach that can be useful when comparing solutions is looking at cyclomatic complexity. Typically (though there are definitely exceptions), a well written function won't have more than a cyclomatic complexity of 10-15. That doesn't mean if it's in that range it's automatically well written, but the inverse is very often useful in that if it's over that number it can probably and should be broken down more.

 

Normally calculating cyclomatic complexity involves breaking down a function into a graph nodes, since it's a measure of graph complexity, but when talking code specifically about you can use this easier counting technique since compiling code is a subset of all possible graphs (since they are unidirectional and always terminate):

1. Start with 1 for the straight path through the routine.
2.  Add 1 for each of the following keywords, or their equivalents: if while repeat for and or
3.  Add 1 for each case in a case statement

for example, as written your acceptButton_Click has a cyclomatic complexity of 8. As to how to view that number, this is from Steve McConnell's Code Complete:

• 0–5 The routine is probably fine.
• 6–10 Start to think about ways to simplify the routine.
• 10+ Break part of the routine into a second routine and call it from the first routine

 

Another more heuristic approach is looking for inline comments. If you feel the need to repeat what the code is doing in a comment, that's a good indicator that you could refactor that logic out into its own logic with a well named method so that you don't need inline documentation to make things clear. This is the concept of writing "self documenting code".

 

Also, if you find your routine is becoming highly nested, that's a good sign to either figure out a more clean way to do that logic, or break out part of the nested logic to de-nesitfy it or at least make it cleaner. Nested code is usually harder to read/maintain.

 

If this were a code review from one of my mentees, I would recommend breaking down the acceptButton_Click with a couple helper methods. One potential opportunity would be to make some that handle the logic for handling the failure case, the success case, as well as a method for retrieving the door index that has the car. Those methods aren't really needed in terms of code complexity, as this function isn't complex, but they help make what the code is doing clearer. 

 

Also, why is it possible that there is no car? That seems like something you ensure so you don't need to handle that error case when doing UI logic

 

[Guest]

M.Yurizaki

Spoiler

1 hour ago, M.Yurizaki said:

It's really a preference thing. If you really want to write 10,000 line functions,

I was wondering if I was going to be getting to close to that point. No need to write an add 1 to value function. 

Quote

there was a general rule of thumb that if your function can't fit on a page, it's too big. Except what's a page?

I do remember hearing about this a long time ago. 

Quote

A screen of text? That's not well defined since even console terminals (which was were the rule of thumb that 80 characters should be the most one line should be) can have a different number of lines (80x20? 80x40?)

Yeah it makes sense. I suppose for my workflow in Visual studio about 1 screen is the upper limits. I expect most collaberators to have a 20~24 inch monitor. It's kinda like saying "As long as you don't have to scroll you're probably fine." 

Quote

Okay, that was sidetracking too much. For me, I start thinking about breaking the function up when:

• it's approaching 100 lines or so
• there's it needs more than three levels of a conditional or loop (or at that point, I need to rethink the logic)
• I'm copying and pasting something

 

If a function is doing multiple things but fits on say a dozen lines of code, that's fine. Like for example, doing some basic preprocessing of text and writing it to a file.

 

Thanks! I was curious as to when functions would be getting too big but those are some good guidelines in my opinion. 

Reniat

Spoiler

1 hour ago, reniat said:

That doesn't mean if it's in that range it's automatically well written, but the inverse is very often useful in that if it's over that number it can probably and should be broken down more.

This seems like a smart gauge. 

Quote

Normally calculating cyclomatic complexity involves breaking down a function into a graph nodes, since it's a measure of graph complexity, but when talking code specifically about you can use this easier counting technique since compiling code is a subset of all possible graphs (since they are unidirectional and always terminate):

1. Start with 1 for the straight path through the routine.
2.  Add 1 for each of the following keywords, or their equivalents: if while repeat for and or
3.  Add 1 for each case in a case statement

for example, as written your acceptButton_Click has a cyclomatic complexity of 8. As to how to view that number, this is from Steve McConnell's Code Complete:

• 0–5 The routine is probably fine.
• 6–10 Start to think about ways to simplify the routine.
• 10+ Break part of the routine into a second routine and call it from the first routine

I like this method of calculating & judgement. I might write it down word for word later. 

Quote

Another more heuristic approach is looking for inline comments. If you feel the need to repeat what the code is doing in a comment, that's a good indicator that you could refactor that logic out into its own logic with a well named method so that you don't need inline documentation to make things clear. This is the concept of writing "self documenting code".

Smart. Helps when you're naming functions to what it does. 

Quote

Also, if you find your routine is becoming highly nested, that's a good sign to either figure out a more clean way to do that logic, or break out part of the nested logic to de-nesitfy it or at least make it cleaner. Nested code is usually harder to read/maintain.

Quote

If this were a code review from one of my mentees, I would recommend breaking down the acceptButton_Click with a couple helper methods. One potential opportunity would be to make some that handle the logic for handling the failure case, the success case, as well as a method for retrieving the door index that has the car. Those methods aren't really needed in terms of code complexity, as this function isn't complex, but they help make what the code is doing clearer. 

I have noticed this but as I usually wrote all the code I looked at in most instances, I should've noticed why these parts were the most time consuming to debug. Definitely will think about this in the future. 

Quote

Also, why is it possible that there is no car? That seems like something you ensure so you don't need to handle that error case when doing UI logic

That was left over from debugging to make sure my logic worked. It works flawlessly, so do you think I should remove the debug variable assignment of negative 1? 
I just never took it out after verifying it worked. 

 

EDIT:
I wish I could pick multiple marked solved. 

[Guest]

5 minutes ago, James Evens said:

Make functions not to small. If you feel it make sense add comments which help to understand the function. To many small functions just case problems.

There is also not a fixed line limit per function. I have functions which basically just connect other functions. The other extreme are several hundreds lines long functions if it makes no sense to split them up.

Yeah that was the main concern with this thread. I was wondering if I'd be making too many functions that could be like "oh I have this problem and have to go searching for all these functions..." 

Or if "This function is enormous. How am I supposed to find where the problem is!?"

[Mira Yurizaki]

I should add, since there are no rules, only guidelines

 

Sometimes you have a function who's only job is to figure out based on some value where to go to next. Basically the entire function is a giant switch-case statement and each case calls another function for processing. A function like this I'm fine even if it gets to be like hundreds of lines of code. At that point you break it up to how many cases you want to handle. 

[Guest]

31 minutes ago, M.Yurizaki said:

I should add, since there are no rules, only guidelines

31 minutes ago, M.Yurizaki said:

Sometimes you have a function who's only job is to figure out based on some value where to go to next. Basically the entire function is a giant switch-case statement and each case calls another function for processing. A function like this I'm fine even if it gets to be like hundreds of lines of code. At that point you break it up to how many cases you want to handle. 

Yeah, I think Reniat had a good explanation of that if you read his post. 

[Unimportant]

17 hours ago, fpo said:

Many programmers say that you should have a function for every thing that has to be done & that each function should only have 1 purpose.

It's a good methodology and I encourage you to try and train yourself to use it consistently for a while. YMMV, of course, but you'll find it tends to grow on you and eventually you'll want to program like that, rather then ask yourself if you really have to.

 

The main purpose is to make self-documenting code that is "human-readable" without extra comments. This helps with maintainability, correctness and debugging. Getting rid of as much comments as possible is important because comments do not tend to age well (that does not mean you should never use comments, but you should not comment what your code can explain for itself). Whenever a program is modified, comments tend to be overlooked and not updated to reflect the new code. How often did you have to work on a program where the comments were outdated and sent you on a wild goose chase ? I've had my fair share.

 

The second purpose is to prevent having to do it anyway in the future. It might be a small project, but most large projects started out small.

 

Let's rewrite your acceptButton_Click function in this way:

private void acceptButton_Click(object sender, EventArgs e)
{
	if (IsCorrectGuess())
	{
		PlayerWins();
	}
	else
	{
		if (IsFirstGuess())
		{
			WrongFirstGuess();
		}
		else
		{
			PlayerLoses();	
		}	
	}
}

We no longer need any comments and the code now reads like plain English. The GuessedCorrect variable became a function that returns true or false.  The function IsFirstGuess stores it's own state inside a class member variable so when you call it the first time it returns true and returns false on subsequent calls automatically. This of course implies this member variable has to be reset by the code that starts a new game.  Having separate functions for PlayerWins, WrongFirstGuess and PlayerLoses groups the actions that should happen to enter this new state together (such as printing the message and disabling the button) in a single place and instills confidence in anyone having to modify the code that those functions are the only path to the new state. It also makes the higher level logic in the acceptButton_click function more readable - "PlayerWins" pretty much sums up what's happening in a single word versus versus code that modifies the text in some status bar and disables a button.

 

Compare both versions and imagine yourself having to work with this code 3 months down the road - which version would you rather work on ?

[jimistephen]

My rule of thumb is try to write code that doesn't need comments. The function names should be as explanatory as possible. Along with that, if you ever write the same code twice it should be a function. The only exception I have to that is if the thing I'm writing is 1 line, I don't want to replace one line of code with one line of a function call, unless I need to call it outside of the file it's in.

[Franck]

22 hours ago, jimistephen said:

My rule of thumb is try to write code that doesn't need comments. The function names should be as explanatory as possible. Along with that, if you ever write the same code twice it should be a function. The only exception I have to that is if the thing I'm writing is 1 line, I don't want to replace one line of code with one line of a function call, unless I need to call it outside of the file it's in.

Opposite to me. If a line of code doesn't have at least 1 comment i feel bad. Even for variables i might have them grouped in small sections and a comment above will tell what they are used for.

 

As for function that is also how i do it plus or less. Sometime i see before writing the code that a small portion i am about to code would be useful for other things then i create a method right away.

[reniat]

1 hour ago, Franck said:

If a line of code doesn't have at least 1 comment i feel bad.

I'd encourage you to look at comments from a different angle.

Comments shouldn't ever repeat what the code is doing verbatim, that's an indicator of unclear poorly self-documenting code. Comments should be used to add meta information, or information about the domain that is not present in the problem scope. If a block or line of code doesn't need any of that external info, than it probably shouldn't need a comment. If you feel it needs a comment, but not for external info reasons, its a good indicator that you could use some refactoring.

 

There is a really good chapter devoted to comments and self-documenting in Code Complete, which is a staple of software development literature, that covers this exact issue of too many vs not enough comments. I'm not going to link it, but I will say that it is very easy to find a free pdf of this book online. The chapter in question is chapter 32.