Skip to content
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.

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

PR-2 #2

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

PR-2 #2

wants to merge 1 commit into from

Conversation

CheezItMan
Copy link

This method calculates a factorial


def function(n)
if n == 0
1

Choose a reason for hiding this comment

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

could be clearer

@@ -0,0 +1,9 @@

def function(n)

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.


def function(n)
if n == 0
1

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.

@@ -0,0 +1,9 @@

def function(n)
if n == 0
Copy link

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

@Galaxylaughing Galaxylaughing left a comment

Choose a reason for hiding this comment

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

ok

@@ -0,0 +1,9 @@

def function(n)

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


def function(n)
if n == 0
1

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)

@@ -0,0 +1,9 @@

def function(n)
if n == 0

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?

Copy link

@raisahv raisahv left a comment

Choose a reason for hiding this comment

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

Approved

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

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)

@@ -0,0 +1,9 @@

def function(n)
Copy link

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?

if n == 0
1
else
n * function(n-1)
Copy link

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.

@@ -0,0 +1,9 @@

def function(n)
Copy link

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


def function(n)
if n == 0
1
Copy link

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

@@ -0,0 +1,9 @@

def function(n)

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

if n == 0
1
else
n * function(n-1)

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)


def function(n)
if n == 0
1

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

if n == 0
1
else
n * function(n-1)

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


def function(n)
if n == 0
1

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


def function(n)
if n == 0
1
Copy link

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

@@ -0,0 +1,9 @@

def function(n)
Copy link

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.