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
[feat/fix/docs]: Improved on conversion code , length variable , asserts added #840
base: master
Are you sure you want to change the base?
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you for your contribution and for your time in making this PR. 👍
Please add documentation and tests as seen in the typical structure of a program.
I changed the code. Added tests and basic documentation. How should I send the file ? |
I pushed the commit. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please make a PR per change. Also, your code is still not up to the repository standards.
Take your time, have a seat, and read them carefully. 🙂
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice work! 😄👍
conversions/binary_to_decimal.c
Outdated
*/ | ||
bool is_binary(intmax_t num) | ||
{ | ||
int remainder = 0; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Consider using uint64_t
for non-negative values (or their appropriate size: uint32_t
, uint16_t
, uint8_t
) or int64_t
for negative values. Requires adding the inttypes.h
library. Check other parts of the code (reference). 🙂
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
uintmax_t has the biggest length for machine running the code. I would prefer it over uint64_t which has a fixed size.
conversions/binary_to_decimal.c
Outdated
// includes | ||
#include <assert.h> | ||
#include <stdbool.h> | ||
#include <stdint.h> | ||
#include <stdio.h> | ||
#include <stdlib.h> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please add a one-line description of what the library/header is for (see the example below).
#include <stdio.h> /// for IO operations
#include <assert.h> /// for assert
@file Co-authored-by: David Leal <halfpacho@gmail.com>
@details Co-authored-by: David Leal <halfpacho@gmail.com>
@details Added ", the" Co-authored-by: David Leal <halfpacho@gmail.com>
Removed a line Co-authored-by: David Leal <halfpacho@gmail.com>
Removed: * Modified Co-authored-by: David Leal <halfpacho@gmail.com>
Suggested: main(void) -> main() Co-authored-by: David Leal <halfpacho@gmail.com>
static void test Co-authored-by: David Leal <halfpacho@gmail.com>
test arg doc Co-authored-by: David Leal <halfpacho@gmail.com>
Comment for test() Co-authored-by: David Leal <halfpacho@gmail.com>
Removed prototypes Co-authored-by: David Leal <halfpacho@gmail.com>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please check the suggestions I made above if they're correct. If so, you can accept them. 🙂
conversions/binary_to_decimal.c
Outdated
|
||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
conversions/binary_to_decimal.c
Outdated
unsigned remainder; | ||
uintmax_t decimal_number = 0, temp = 1; | ||
|
||
int length = num_len(UINTMAX_MAX) - 1; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Consider using uint64_t
for non-negative values (or their appropriate size: uint32_t
, uint16_t
, uint8_t
) or int64_t
for negative values. Requires using the inttypes.h
library. Check other parts of the code (reference). 🙂
corrected @return Co-authored-by: David Leal <halfpacho@gmail.com>
Removed space Co-authored-by: David Leal <halfpacho@gmail.com>
Removed // includes Co-authored-by: David Leal <halfpacho@gmail.com>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Almost there! 😄
conversions/binary_to_decimal.c
Outdated
uint8_t remainder; | ||
uintmax_t decimal_number = 0; | ||
uint32_t temp = 1; | ||
|
||
uint16_t length = num_len(UINTMAX_MAX) - 1; | ||
|
||
assert(num_len(number) <= length); | ||
assert(is_binary(number)); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Wrong indentation. Please fix it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let's see if it works.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Almost there! 😄
Removed a word Co-authored-by: David Leal <halfpacho@gmail.com>
it to that Co-authored-by: David Leal <halfpacho@gmail.com>
Added number Co-authored-by: David Leal <halfpacho@gmail.com>
printf indent Co-authored-by: David Leal <halfpacho@gmail.com>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Almost there! 😄
conversions/binary_to_decimal.c
Outdated
remainder = num % 10; | ||
if (remainder == 0 || remainder == 1) { | ||
num /= 10; | ||
continue; | ||
} else | ||
return false; | ||
} | ||
return true; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please add a bit of documentation here of what this code does.
Added 'number' Co-authored-by: David Leal <halfpacho@gmail.com>
Added `` Co-authored-by: David Leal <halfpacho@gmail.com>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Amazing work! Thank you for your dedication and contributions to our community! 😄🎉👍
Description of Change
References
Checklist
Notes: