Base Conversion in Node.js

Question

I am doing a program for my APCS class in Node.js to practice node, but I am unsure if I did everything the best way possible.

The assignment is to take the number and base as user input and then perform the conversion and print it out.

Here is the code:

const readline = require('readline');
const rl = readline.createInterface({
 input: process.stdin,
 output: process.stdout
});
const BASE_CHARACTERS = "0123456789ABCDEFGHIJKLMNOPQRSTUVWXYZabcdefghijklmnopqrstuvwxyz/=";
exports.pa11 = function() {
 rl.question("Enter number: ", number => {
 rl.question("Enter base: ", base => {
 if (base <= 1) throw "Base must be greater than 1";
 else if (base > 64) throw "Base must be less than or equal to 64";
 const numberOriginal = number, baseOriginal = base;
 const negative = number < 0;
 if (negative) number = -number;
 let x = "";
 while (number > 0) {
 let digit = number % base;
 number -= digit;
 number /= base;
 x = BASE_CHARACTERS.charAt(digit) + x;
 }
 if (negative) x = "-" + x;
 console.log(numberOriginal, "in base", baseOriginal, "is", x);
 rl.close();
 process.exit();
 });
 });
};

Answer 1

Testing, vetting, and redundancy.

First you have not clearly defined the requirement. Base64 is an encoding used to convert bit/byte streams for safe transport. It is not the same as n-based number systems.

I will assume you want n-based number systems

Your code does not do a conversion that makes any sense at all, you should test your code before you release it.

Anyways, the easy solution up to base 36 is:

if(base <= 36){
 value = parseInt(number, base);
}

but I don't think that is what the lesson is designed to teach. So here is my assessment, and it would be a fail as it stands.

Throw Error objects not strings.

When you throw you should always throw an error object. One of the appropriate Error object types see the MDN documentation for Error.

You may not be handling the thrown errors and if the error handler does not expect a string it to may throw an error attempting to access non existing properties.

Compound declaration with assignment.

Generally I don't like compound declarations that have assignments, though I do make exception to last item assignment. I find it even less readable if you are inconsistent in the use.

Example of compound declaration and assignment:

// Bad
var a = 0, b = 1, c;
// or
var a = 0,
 b = 1,
 c;
// Good
var a, b, c;
a = 0;
b = 1;

You have:

// bad one line is compound and then you have a single
const numberOriginal = number, baseOriginal = base;
const negative = number < 0;

You should have:

const numberOriginal = number;
const baseOriginal = base;
const negative = number < 0;

Dont create needless noise.

You create the const baseOriginal and assign it the value of base yet at no point do you modify base before you use baseOriginal making it redundant. This just adds complexity where it is not needed making code harder to read and modify.

Show you understand let, var, and const

There is much debate about the use of let, var, and const with some schools of though opting for the use of only let and const.

The problem with this is that when I look at code I am in part assessing the writer's level of expertise and if I see a declaration out of place I wonder if that coder knows what he/she is doing. Once I lose trust in the code the marks fall off much quicker.

I understand that some teach the use of let instead of var - no harm done, right? No!!! var and let create very different behaviours and you should use them appropriately. Creating a block-scoped variable in function scope is incorrect. If you declare in-function scope then you should use var to show you know you are using function scope.

If the variable is never modified you should use const and you should place it as high as possible in the scope that it is needed.

Use let only inside blocks (With the exception of for loops)

Vars are hoisted and thus you should put var declarations where they are declared, first lines after the function declaration. If you don't then it is hard to know what has been defined, and the person reading the code must search for the var. This leads to errors and bugs.

So loud in here.

Noise is anything that adds to the source but serves not purpose. Good code has zero noise (with the exception of style dictated noise, though a note should always provide details on uncommon style and reason for use)

You have:

if (base <= 1) throw "Base must be greater than 1";
else if (base > 64) throw "Base must be less than or equal to 64";

If the first statement is true then the code is terminated at the throw. That makes the else redundant and just noise, so don't use it.

if (base <= 1) { throw new Error("Base must be greater than 1") }
if (base > 64) { throw new Error( "Base must be less than or equal to 64") }

Don't use blockless blocks.

Blockless blocks are a major source of bugs. Not only are they bugs waiting to happen, they are particularly hard to spot. There is a special place in the hell of a billion bugs for the person that introduced that in to languages.

// bad, no worse than bad.
if (something) doThat();
// or even worse than worse than bad.
if (something) 
 doThat();
// and an extra notch of bad
if (something) 
doThat()
// Good
if (something) { doThat(); }
// or
if (something) { doThat() }
// or
if (something) { 
 doThat();
}

Vet your input

The two inputs from the question are strings. You neglect to test if base is NaN, and you do not check whether number contains only valid characters.

if(isNaN(base)) throw new TypeError("Base must be a number from 2-64 inclusive.");

The number should also be tested but as that will depend on the value of base, and to do a proper test means iterating all the characters that you will do in the function logic you should add the error in the while loop when you are doing the conversion.

Be type aware

The argument number is a string and as it is variable base, attempting to coerce it to a number can fail.

You have:

const negative = number < 0;
if (negative) number = -number; 

This will not work if number = "-a"

Check for the sign as the first character of the string:

const negative = number[0] === "-";
if (negative) { number = number.substr(1) } // remove the minus sign

Did you test your code?

Looking at the function I instantly see that it will not work. Your logic is completely wrong. You should have known that because you should have tested it and seen the wrong output for the given input. Always test your code before you let it out of your sight. You don't write half sentences nor should you write broken code. Code is broken until you have tested it not before.

The function as you have it does not allow easy testing. You should separate out the conversion so that you can automatically test it without having to modify your code.

Dont blindly follow the lead.

There is a tendency for node programmers to use nondescript variable names. This is a very bad trend and should be stopped. Out of context what is rl, or fs, only those in the know will understand. If I did not know better, the persistence of these abbreviations in nodeJS code would suggest a form of deliberate elitism.

Use descriptive names for variable. Just because everyone uses a particular abbreviation does not make it a good choice of name.

// bad
const rl = ...
// good
const readLineInterface = ...

This allows everyone to understand what the variable is, not just those in the know.

The rewrite

// Style note. Single line blocks do not include a ; before the closing } this is to comply with jsLint rule
// semicolons should be followed by a new line.
const readline = require('readline');
exports.pa11 = function () {
 const readLineInterface = readline.createInterface({
 input: process.stdin,
 output: process.stdout
 });
 function convertNumber(number, base){
 var value, digit, charIndex;
 const BASE_CHARACTERS = "0123456789ABCDEFGHIJKLMNOPQRSTUVWXYZabcdefghijklmnopqrstuvwxyz/=";
 if (isNaN(base)) { throw new TypeError("Base must be a number from 2 - 64") } 
 if (base < 2 || base > 64) { throw new RangeError("Base must be a number from 2 - 64") }
 const numberOrg = number;
 const negative = number[0] === "-";
 if (negative) { number = number.substr(1) } // remove the minus sign
 charIndex = number.length; // start from least significant digit
 digit = 0; // the current place of digit
 value = 0; // the base ten value 
 while (charIndex--) {
 const index = BASE_CHARACTERS.indexOf(number[charIndex]);
 if (index === -1 || index >= base) {
 throw new TypeError(`Number ${numberOrg} is not a base '${base}' number.`);
 }
 value += Math.pow(base, digit++);
 }
 value *= negative ? -1 : 1; // if negative negate result
 console.log(`The number ${numberOrg}, in base ${base} has the value ${value} in base 10.`);
 return value;
 }
 readLineInterface.question("Enter number: ", number => {
 readLineInterface.question("Enter base: ", base => {
 convertNumber(number, base);
 readLineInterface.close();
 process.exit();
 });
 });
};

Note

Many markers use automatic plagiarism systems that search the web for copied content. If you copy the above code as-is it is highly likely that your code will be flagged by such systems. You will have to modify it rather than copy and past it.

I have left one missing error check. If the input number string evaluates to a value that can not be accurately expressed as a Javascript Number then the result will be incorrect. You should test the result and ensure it is in safe bounds. Hint: look up Number.MAX_SAFE_INTEGER and its negative counterpart.

• Javier García: Great review, blind guy :)
• Zach Hilman: Thanks for the help! I am actually doing this on my own as my class is Java but I already know that. This happens to be my first js program ever. Do you have any recommendations on a site or book for best practices/learning js? Also I did not know there was a difference between let and car, could you tell me? Thanks again!
• Blindman67: @ZachHilman the difference between var and let are mostly to do with scope,let (and const) are block scope, var is function scope. Scope is an important concept that you will need to understand before you can progress to the difference between let and var. There is little room in the comments to explain so a good reference and source of examples and tutorials is MDN developer.mozilla.org/en-US/docs/Web/JavaScript.
• Gerrit0: I'm not entirely sure I agree with mixing let and var. By doing so I have to think more about which I really mean. It is very rare to need var for its function scoped capabilities. (I can't actually think of a place it is needed right now). Lastly, I know it's not the focus of the assignment, but parseInt(str, radix) is worth mentioning :)
• Blindman67: @Gerrit0 I mention 'parseInt' in the first section as the easy solution. If you use 'let' in function scope you have created a function scoped variable and depending on where you define it you have also introduced a function scoped variable with a dead zone. It's not if you need a function scoped variable or not, it's about using the best type for the scope you are in. Though each may have their own reasoning I believe my argument sound, and I'll be dragged over hot coals before using a VB token if I don't have to, what next... then :P

Answer 2

I would separate the logic to do the conversion into it's own function separate from the i/o. i.e.

function convert(number, base) {
 ...
}
exports.pa11 = function() {
 rl.question("Enter number: ", number => {
 rl.question("Enter base: ", base => {
 const result = convert(number, base);
 ...

This makes it easier to perform automated testing on your function and to call it from other code (which is usually the goal).

I would also make the logic case-insensitive for bases less than 37.