PR-2 by CheezItMan · Pull Request #2 · Ada-C12/code-reviews

CheezItMan · 2020-02-06T18:50:37Z

This method calculates a factorial

brilatimer · 2020-02-06T21:48:02Z

+
+def function(n)
+  if n == 0
+    1


could be clearer

dtaylor73 · 2020-02-06T21:42:02Z

@@ -0,0 +1,9 @@
+
+def function(n)


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

dtaylor73 · 2020-02-06T21:44:17Z

+
+def function(n)
+  if n == 0
+    1


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

jaitch · 2020-02-06T21:40:37Z

@@ -0,0 +1,9 @@
+
+def function(n)
+  if n == 0


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

Galaxylaughing

ok

Galaxylaughing · 2020-02-06T21:44:03Z

@@ -0,0 +1,9 @@
+
+def function(n)


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

Galaxylaughing · 2020-02-06T21:44:45Z

+
+def function(n)
+  if n == 0
+    1


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

Galaxylaughing · 2020-02-06T21:45:28Z

@@ -0,0 +1,9 @@
+
+def function(n)
+  if n == 0


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

raisahv

Approved

raisahv · 2020-02-06T21:43:45Z

+    1
+  else
+    n * function(n-1)
+  end


can shorten this to

Suggested change

end

n == 0 ? 1 : n * function(n-1)

dtingg · 2020-02-06T21:46:41Z

@@ -0,0 +1,9 @@
+
+def function(n)


Maybe rename this function to factorial?

dtingg · 2020-02-06T21:47:07Z

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


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

north108 · 2020-02-06T21:44:11Z

@@ -0,0 +1,9 @@
+
+def function(n)


function naming should be more clear as to what it does

north108 · 2020-02-06T21:44:48Z

+
+def function(n)
+  if n == 0
+    1


add return to be more explicit

sun-alice · 2020-02-06T21:44:18Z

@@ -0,0 +1,9 @@
+
+def function(n)


name this something more elaborate

sun-alice · 2020-02-06T21:46:20Z

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


Suggested change

n * function(n-1)

return n * function(n-1)

sun-alice · 2020-02-06T22:15:38Z

+
+def function(n)
+  if n == 0
+    1


Suggested change

1

return 1

emilyvomacka · 2020-02-06T21:42:44Z

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


Should handle the initial case of n < 0

emilyvomacka · 2020-02-06T21:51:40Z

+
+def function(n)
+  if n == 0
+    1


could add "return" to make it clearer

mdove92 · 2020-02-06T21:48:31Z

+
+def function(n)
+  if n == 0
+    1


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

mdove92 · 2020-02-06T21:49:11Z

@@ -0,0 +1,9 @@
+
+def function(n)


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.

function method added

1ecb4a0

brilatimer reviewed Feb 6, 2020

View reviewed changes

Comment thread function.rb

def function(n)

if n == 0

1

Copy link
Copy Markdown

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

dtaylor73 reviewed Feb 6, 2020

View reviewed changes

jaitch reviewed Feb 6, 2020

View reviewed changes

Comment thread function.rb

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

def function(n)

if n == 0

Copy link
Copy Markdown

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"?

Galaxylaughing reviewed Feb 6, 2020

View reviewed changes

raisahv reviewed Feb 6, 2020

View reviewed changes

dtingg reviewed Feb 6, 2020

View reviewed changes

north108 reviewed Feb 6, 2020

View reviewed changes

sun-alice reviewed Feb 6, 2020

View reviewed changes

emilyvomacka reviewed Feb 6, 2020

View reviewed changes

mdove92 reviewed Feb 6, 2020

View reviewed changes

Conversation

CheezItMan commented Feb 6, 2020

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Galaxylaughing left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

raisahv left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

11 participants