add more functions to DecimalToBinary.js file from Conversions directory #1074

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

Jump to bottom

Closed

AanthereX wants to merge 1 commit into TheAlgorithms:master from AanthereX:add-more-functions-to-conversion-decimal-to-binary

Closed

add more functions to DecimalToBinary.js file from Conversions directory #1074

AanthereX wants to merge 1 commit into TheAlgorithms:master from AanthereX:add-more-functions-to-conversion-decimal-to-binary

Conversation

AanthereX

@AanthereX


 add more functions to DecimalToBinary.js file from Conversions directory

322a67c

@AanthereX AanthereX requested review from raklaptudirm and appgurueu as code owners

August 2, 2022 13:08

@lgtm-com

Copy link

lgtm-com bot commented Aug 2, 2022

This pull request introduces 1 alert when merging 322a67c into 1a089cc - view on LGTM.com

new alerts:

1 for Unused variable, import, function or class

@appgurueu appgurueu added the algorithm label

Aug 3, 2022

appgurueu

appgurueu requested changes

Aug 3, 2022

View reviewed changes

Copy link

Collaborator

@appgurueu appgurueu left a comment

Choose a reason for hiding this comment

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

Overall questionable: Second method just lets JS builtins do the job, third one has poor code quality and is mostly a duplicate of the first one. Leaning towards rejecting this - it probably has to be redone pretty much entirely, if it should be redone at all.

Conversions/DecimalToBinary.js

// using .toString() method

function decimalToBinary_2(dec) {

return dec.toString(2)

Copy link

Collaborator

@appgurueu appgurueu Aug 3, 2022

Choose a reason for hiding this comment

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

This feels like cheating: The purpose of this repo is to demonstrate how this could be implemented without relying on libraries (including the standard library). Perhaps note the existence of a suitable stdlib func in a comment?

Conversions/DecimalToBinary.js

@@ -16,4 +38,4 @@ export { decimalToBinary }

// '111'

// > decimalToBinary(35)

// '100011'

Copy link

Collaborator

@appgurueu appgurueu Aug 3, 2022

Choose a reason for hiding this comment

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

Please convert to proper tests instead of removing the trailing newline (?)

Conversions/DecimalToBinary.js

function decimalToBinary_3(dec) {

let bin = 0;

let rem, i = 1,

step = 1;

Copy link

Collaborator

@appgurueu appgurueu Aug 3, 2022

Choose a reason for hiding this comment

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

Unused variable.

Conversions/DecimalToBinary.js

step = 1;

while (dec != 0) {

rem = dec % 2;

dec = parseInt(dec / 2);

Copy link

Collaborator

@appgurueu appgurueu Aug 3, 2022

Choose a reason for hiding this comment

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

Looks like a hacky reimplementation of decimalToBinary to me, except instead of using an array to store the digits this uses a base 10 number which may then be stringified in base 10 to produce the desired binary number.

The parseInt hack to achieve floor division rather than using dec >>= 1 is a clear downgrade. You could consider Math.floor(dec / 2) for readability, but relying on parseInt to perform flooring is hacky IMO.