Implementing Factorial using recursion #1370

sricharan200 · 2023-11-15T04:08:45Z

Description of Change

References

Checklist

Added file name matches File name guidelines
Added tests and example, test must pass
PR title follows semantic commit guidelines
Search previous suggestions before making a new one, as yours may be a duplicate.
I acknowledge that all my contributions will be made under the project's license.

Notes: Hey, I have raised a pull request. This is part of my coursework assignment. Kindly, look into it. Thank you.

realstealthninja

thank you for submitting next time please ensure you create separate pull requests for each file change ❤️

realstealthninja · 2023-11-15T13:45:25Z

sorting/radix_sort.c

+    displayArray(array5, length5);
+
+    return 0;
+}


Suggested change

}

}

math/factorial_recursion.c

+    return factResults[sequenceNumber];
+}
+
+int main() {


math/factorial_recursion.c

@@ -0,0 +1,33 @@
+#include <stdio.h>


realstealthninja · 2023-11-15T13:50:57Z

math/factorial_recursion.c

+
+unsigned long long factResults[MAX_LIMIT] = {0};
+
+unsigned long long computeFactorial(int sequenceNumber) {


using lengthy data types is not recommended
use the _t types from stdint.h

#include<stdint.h> uint64_t computeFactorial(...)

Suggested change

unsigned long long computeFactorial(int sequenceNumber) {

uint64_t computeFactorial(int sequenceNumber) {

math/factorial_recursion.c

@@ -0,0 +1,33 @@
+#include <stdio.h>


realstealthninja · 2023-11-15T13:54:33Z

sorting/radix_sort.c

+    int array1[] = {121, 432, 564, 23, 1, 45, 788};
+    int length1 = sizeof(array1) / sizeof(array1[0]);
+    int array2[] = {95, 90, 85, 80, 75, 70};
+    int length2 = sizeof(array2) / sizeof(array2[0]);


Suggested change

int length2 = sizeof(array2) / sizeof(array2[0]);

int length2 = sizeof(array2) / sizeof(int);

realstealthninja · 2023-11-15T13:54:46Z

sorting/radix_sort.c

+    int array2[] = {95, 90, 85, 80, 75, 70};
+    int length2 = sizeof(array2) / sizeof(array2[0]);
+    int array3[] = {333, 287, 911, 462, 786};
+    int length3 = sizeof(array3) / sizeof(array3[0]);


Suggested change

int length3 = sizeof(array3) / sizeof(array3[0]);

int length3 = sizeof(array3) / sizeof(int);

realstealthninja · 2023-11-15T13:55:02Z

sorting/radix_sort.c

+    int array3[] = {333, 287, 911, 462, 786};
+    int length3 = sizeof(array3) / sizeof(array3[0]);
+    int array4[] = {5, 3, 8, 1, 2, 9, 4, 7, 6};
+    int length4 = sizeof(array4) / sizeof(array4[0]);


Suggested change

int length4 = sizeof(array4) / sizeof(array4[0]);

int length4 = sizeof(array4) / sizeof(int);

realstealthninja · 2023-11-15T13:55:28Z

sorting/radix_sort.c

+    int array4[] = {5, 3, 8, 1, 2, 9, 4, 7, 6};
+    int length4 = sizeof(array4) / sizeof(array4[0]);
+    int array5[] = {12, 34, 12, 45, 67, 45, 34};
+    int length5 = sizeof(array5) / sizeof(array5[0]);


Suggested change

int length5 = sizeof(array5) / sizeof(array5[0]);

int length5 = sizeof(array5) / sizeof(int);

realstealthninja · 2023-11-15T13:57:21Z

sorting/radix_sort.c

+    int length4 = sizeof(array4) / sizeof(array4[0]);
+    int array5[] = {12, 34, 12, 45, 67, 45, 34};
+    int length5 = sizeof(array5) / sizeof(array5[0]);
+


if you are going for speed with the test of this algorithm you could just do

float size_of_int = 1/sizeof(int); // then multiply this value instead of divding over and over again // remember multiplication is always faster than division int length1 = sizeof(array1) * size_of_int; int length2 = sizeof(array2) * size_of_int; int length3 = sizeof(array3) * size_of_int; int length4 = sizeof(array4) * size_of_int; int length5 = sizeof(array5) * size_of_int;

sricharan200 · 2023-11-15T23:14:23Z

I have made the required changes for factorial recursion.
As I have raised pull request for factorial using recursion. Kindly look into it. Thank you

realstealthninja

please mark things youve completed via the resolved button thats below each suggestion

realstealthninja · 2023-11-16T13:00:18Z

sorting/radix_sort.c

remove this file from this pr

realstealthninja · 2023-11-16T13:06:02Z

math/factorial_recursion.c

+    assert(computeFactorial(10) == 3628800);
+
+}
+


all functions must be documented this is because when doxygen generates documentation
it pulls from your inline documentation

refer to the format below as well as the suggestion:

/* * @brief <explanation of function in a sentence> * @param <parameter name> <explanation of parameter> * @returns <return type> */ type identifier(type param_name) {...}

Suggested change

/*

* @brief main function

* @returns 0 on successful exit

*/

Made all the required changes. Thank you.

realstealthninja · 2023-11-16T13:07:03Z

math/factorial_recursion.c

+    - Memoization: https://www.geeksforgeeks.org/memoization-1d-2d-and-3d/
+*/
+
+#include <stdio.h>


it would be preferable if you document the headers as well

#include<header.h> /// <reason for using header>

I have made all the required changes. Kindly look into it. Thank you.

@realstealthninja, can you please review the pull request?

realstealthninja

very good

sricharan200 · 2023-11-27T15:13:13Z

@realstealthninja Thanks. Is the pull request merged?

realstealthninja · 2023-11-27T15:15:40Z

Sadly not I'm afraid. You see the maintainers are all busy with their lives so we're pretty much stuck until they're able to review pull requests. Please be patient

sricharan200 · 2023-11-27T15:17:04Z

Okay. As, Github contributions are a part of my coursework assignment. I am supposed to finish it by the first week of December.

sricharan200 · 2023-11-28T15:50:45Z

Hi, @realstealthninja , Can you please let the other reviewers know about the pull request review?

PS: This is a part of my coursework assignment that counts towards the final grade. This assignment has to be completed by first week of December.

realstealthninja · 2023-11-28T16:01:32Z

Will try to get their attention no promises though. Thank you for your contributions

vil02

I do not have the power to merge it, but there are some issues with this code.

vil02 · 2023-11-28T21:26:09Z

math/factorial_recursion.c

+#include <stdint.h> /// Standardized integer data types for improved portability.
+#include <assert.h> /// Assertions for testing purposes.
+
+#define MAX_LIMIT 200


Why 200? What is the highest input, for which the factorial will fit into uint64_t?

vil02 · 2023-11-28T21:27:49Z

math/factorial_recursion.c

+ * @param sequenceNumber The non-negative integer for which to compute the factorial.
+ * @return The factorial of the input integer.
+ */
+uint64_t computeFactorial(int sequenceNumber) {


Suggested change

uint64_t computeFactorial(int sequenceNumber) {

uint64_t computeFactorial(unsinged sequenceNumber) {

It does not make sense to run computeFactorial(-1), right? Why not to use unisigned as input type?

You can also go for:

uint64_t computeFactorial(const unsinged sequenceNumber) {

vil02 · 2023-11-28T21:28:49Z

math/factorial_recursion.c

+    if (sequenceNumber == 0) {
+        return 1;
+    }


You are wasting one slot in the factResults.

vil02 · 2023-11-28T21:29:34Z

math/factorial_recursion.c

+    if (sequenceNumber == 0) {
+        return 1;
+    }
+    if (factResults[sequenceNumber] != 0) {


What will happen if you call computeFactorial(1000) or computeFactorial(-1000)?

vil02 · 2023-11-28T21:35:12Z

math/factorial_recursion.c

+    if (factResults[sequenceNumber] != 0) {
+        return factResults[sequenceNumber];
+    }
+    factResults[sequenceNumber] = sequenceNumber * computeFactorial(sequenceNumber - 1);


You have a conversion to 'uint64_t' from 'int' may change the sign of the result - this might be much more serious than it looks at first. Either make sequenceNumber unsinged or change this line to:

factResults[sequenceNumber] = (uint64_t)(sequenceNumber) * computeFactorial(sequenceNumber - 1);

vil02 · 2023-11-28T21:38:07Z

math/factorial_recursion.c

+/**
+ * @brief Tests the 'computeFactorial' function by comparing its results with known factorial values.
+ */
+void testFactorial() {


Suggested change

void testFactorial() {

void testFactorial(void) {

cf. -Wstrict-prototypes.

vil02 · 2023-11-28T21:38:30Z

math/factorial_recursion.c

+ * @brief Main function to test the factorial calculation.
+ * @return 0 on success.
+ */
+int main() {


Suggested change

int main() {

int main(void) {

vil02 · 2023-11-28T21:40:57Z

math/factorial_recursion.c

+
+    int testNumber;
+    testNumber = 5;
+    printf("The factorial of %d is %llu\n", testNumber, computeFactorial(testNumber));


If you want to be super correct, then change this line to:

Suggested change

printf("The factorial of %d is %llu\n", testNumber, computeFactorial(testNumber));

printf("The factorial of %d is %" PRIu64 "\n", testNumber, computeFactorial(testNumber));

Make sure to:

#include <inttypes.h>

vil02 · 2023-11-28T21:45:45Z

math/factorial_recursion.c

+
+#define MAX_LIMIT 200
+
+uint64_t factResults[MAX_LIMIT] = {0};


Why not to make this a static variable inside computeFactorial?

vil02 · 2023-11-28T21:47:41Z

sorting/radix_sort.c

Is there a reason why are you deleting this file?

github-actions · 2023-12-29T00:18:03Z

This pull request has been automatically marked as abandoned because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

github-actions · 2024-01-05T00:21:51Z

Please ping one of the maintainers once you commit the changes requested or make improvements on the code. If this is not the case and you need some help, feel free to ask for help in our Gitter channel or our Discord server. Thank you for your contributions!

sricharan200 added 4 commits November 14, 2023 22:05

Add files via upload

13e10f9

Add files via upload

bdbb88f

Delete math/factorial_recursion.c

78aea46

Add files via upload

5faf1ed

sricharan200 requested review from Panquesito7, tjgurwara99 and alexpantyukhin as code owners November 15, 2023 04:08

sricharan200 added 2 commits November 15, 2023 01:21

Delete sorting/radix_sort.c

d9c1639

Add files via upload

c3d4082

realstealthninja requested changes Nov 15, 2023

View reviewed changes

Update factorial_recursion.c

e2b34c3

realstealthninja requested changes Nov 16, 2023

View reviewed changes

sricharan200 added 4 commits November 16, 2023 08:12

Delete sorting/radix_sort.c

909c3eb

Update factorial_recursion.c

b26ddb8

Update factorial_recursion.c

6c6c480

Update factorial_recursion.c

205c10e

realstealthninja approved these changes Nov 27, 2023

View reviewed changes

alexpantyukhin approved these changes Nov 28, 2023

View reviewed changes

vil02 suggested changes Nov 28, 2023

View reviewed changes

github-actions bot added the Stale label Dec 29, 2023

github-actions bot closed this Jan 5, 2024


		unsigned long long factResults[MAX_LIMIT] = {0};

		unsigned long long computeFactorial(int sequenceNumber) {

	unsigned long long computeFactorial(int sequenceNumber) {
	uint64_t computeFactorial(int sequenceNumber) {

	int length2 = sizeof(array2) / sizeof(array2[0]);
	int length2 = sizeof(array2) / sizeof(int);

	int length3 = sizeof(array3) / sizeof(array3[0]);
	int length3 = sizeof(array3) / sizeof(int);

	int length4 = sizeof(array4) / sizeof(array4[0]);
	int length4 = sizeof(array4) / sizeof(int);

	int length5 = sizeof(array5) / sizeof(array5[0]);
	int length5 = sizeof(array5) / sizeof(int);

+/*
+ * @brief main function
+ * @returns 0 on successful exit
+ */

	uint64_t computeFactorial(int sequenceNumber) {
	uint64_t computeFactorial(unsinged sequenceNumber) {

	printf("The factorial of %d is %llu\n", testNumber, computeFactorial(testNumber));
	printf("The factorial of %d is %" PRIu64 "\n", testNumber, computeFactorial(testNumber));

Implementing Factorial using recursion #1370

Implementing Factorial using recursion #1370

Conversation

sricharan200 commented Nov 15, 2023 • edited Loading

Description of Change

References

Checklist

realstealthninja left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

This comment was marked as resolved.

This comment was marked as resolved.

Choose a reason for hiding this comment

This comment was marked as resolved.

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sricharan200 commented Nov 15, 2023

realstealthninja left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

realstealthninja left a comment

Choose a reason for hiding this comment

sricharan200 commented Nov 27, 2023

realstealthninja commented Nov 27, 2023

sricharan200 commented Nov 27, 2023

sricharan200 commented Nov 28, 2023

realstealthninja commented Nov 28, 2023

vil02 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

github-actions bot commented Dec 29, 2023

github-actions bot commented Jan 5, 2024

sricharan200 commented Nov 15, 2023 •

edited

Loading

realstealthninja left a comment •

edited

Loading