Skip to content

PR-2 #2

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
CheezItMan wants to merge 1 commit into
base: master
Choose a base branch
from
Open

PR-2 #2

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
9 changes: 9 additions & 0 deletions function.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,9 @@

def function(n)
Copy link
Copy Markdown

@dtaylor73 dtaylor73 Feb 6, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let's make this function dryer. We could write a ternary statement instead of spreading the code out into multiple lines.

Copy link
Copy Markdown

@Galaxylaughing Galaxylaughing Feb 6, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

function doesn't seem like a meaningful name; it should probably be called 'factorial' or something

Copy link
Copy Markdown

@north108 north108 Feb 6, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

function naming should be more clear as to what it does

Copy link
Copy Markdown

@sun-alice sun-alice Feb 6, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

name this something more elaborate

Copy link
Copy Markdown

@dtingg dtingg Feb 6, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe rename this function to factorial?

Copy link
Copy Markdown

@mdove92 mdove92 Feb 6, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This should have a more descriptive name. Function dos not let us know what to use it for, and may be a reserved word in ruby.

if n == 0
Copy link
Copy Markdown

@jaitch jaitch Feb 6, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Did you mean to include "return" before the "1"?

Copy link
Copy Markdown

@Galaxylaughing Galaxylaughing Feb 6, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

if n is a float, this function might go on forever since it will never == zero. Perhaps <= would be better?

1
Copy link
Copy Markdown

@dtaylor73 dtaylor73 Feb 6, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let's write 'puts' in front of the code we want to print to the screen.

Copy link
Copy Markdown

@Galaxylaughing Galaxylaughing Feb 6, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think you need to explicitly return 1 here and, on line 6, return n * function(n-1)

Copy link
Copy Markdown

@north108 north108 Feb 6, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

add return to be more explicit

Copy link
Copy Markdown

@brilatimer brilatimer Feb 6, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

could be clearer

Copy link
Copy Markdown

@mdove92 mdove92 Feb 6, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What is the 1 supposed to be set to? Are you trying to return it? Please write it out

Copy link
Copy Markdown

@emilyvomacka emilyvomacka Feb 6, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

could add "return" to make it clearer

Copy link
Copy Markdown

@sun-alice sun-alice Feb 6, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
1
return 1

else
n * function(n-1)
Copy link
Copy Markdown

@emilyvomacka emilyvomacka Feb 6, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should handle the initial case of n < 0

Copy link
Copy Markdown

@sun-alice sun-alice Feb 6, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
n * function(n-1)
return n * function(n-1)

Copy link
Copy Markdown

@dtingg dtingg Feb 6, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If you change the name of the function, you have to change it here too.

end
Copy link
Copy Markdown

@raisahv raisahv Feb 6, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

can shorten this to

Suggested change
end
n == 0 ? 1 : n * function(n-1)

end