Fractional binary number to decimal number

Question

Write and test your own double abc (char * b) function in the program, taking in the form of a string the fractional binary number of the form "1101.101" and returning an equal decimal number (here 1 * 2 ^ 3 + 1 * 2 ^ 2 + 0 * 2 ^ 1 + 1 * 2 ^ 0 + 1 * 2 ^ -1 + 0 * 2 ^ -2 + 1 * 2 ^ -3 =わ 8 +たす 4 +たす 0 +たす 1 +たす 0.5 + 0 + 0.125 = 13.625).

The function should return the special value -1 if an incorrect fractional binary number is entered (e.g. containing a different digit than 0, 1 or more than one period '.').

My solution:

#include <iostream>
#include <cmath>
using namespace std;
double abc(char* b);
int main() {
cout << abc("1101.101");
return 0;
}
double abc(char* b){
int i = 0;
int a = 0;
int dot = 0;
double sum=0;
int c = 0;
do{
 if ((b[a]=='1')||(b[a]=='0')||(b[a]=='.')){
 a++;
 if(b[a]=='.'){
 dot++;
 }
 if(dot==2){
 return -1;
 }
 } else {
 return -1;
 }
} while(b[a]!='0円');
while(b[i]!='.'){
 i++;
}
while(i!=0) {
 if(b[c]=='1'){
 sum = sum + pow (2, i-1);
 }
 c++;
 i--;
}
while(b[i]!='.'){
 i++;
}
if(b[i]=='.'){
 int m = 0;
 while(b[i]!='0円'){
 if(b[i]=='1'){
 sum = sum + pow (2, m);
 }
 m = m - 1;
 i++;
 }
}
return sum;
}

Problem: Everything is fine, it gives me 13.625, but there is a lot of code. Is there any faster way to solve this exercise?

Answer 1

Here are some thoughts about your code:

• Don't use using namespace std. This is considered bad practice and you can find many reasons on the internet why this is the case (for example here). You can, for example write std::cout instead.

• You should use variable-names that tell the person who reads the code the purpose of the variable.

• To improve the legibility of your code, you really should use indentation.

• To further improve legibility, you can leave spaces between operators.

• I made your code a bit shorter by avoiding redundant code. You could even make it a lot shorter still, but I wanted to maintain your code structure.

• Finally, you should have a look at edge cases: Empty strings, strings that only contain "." and strings that don't have positions after/before the decimal point (for example 101 = 5 or .101 = 0.625).

The result looks like this:

#include <iostream>
#include <cmath>
#include <cstring>
double abc(char* b);
int main() {
 std::cout << abc(".101") << "\n";
 return 0;
}
double abc(char* b){
 if(strcmp("", b) == 0 || strcmp(".", b) == 0) { //Empty String or only "."
 return -1;
 }
 if(b[0] == '.') { //No positions before the decimal point
 int length = std::strlen(b);
 char* newB = new char[length + 1];
 newB[0] = '0';
 for(int k = 0; k < length; k++) {
 newB[k + 1] = b[k];
 }
 b = newB;
 }
 int countDots = 0;
 int index = 0;
 int dotIndex = -1;
 do{
 if ((b[index] == '1') || (b[index] == '0') || (b[index] == '.')){
 index++;
 if(b[index] == '.'){
 countDots++;
 dotIndex = index;
 }
 if(countDots == 2){
 return -1;
 }
 } 
 else {
 return -1;
 }
 } while(b[index] != '0円');
 if(dotIndex == -1) { //No dot found, but not empty string, so you have a natural number
 int length = std::strlen(b);
 char* newB = new char[length + 2];
 for(int k = 0; k < length; k++) {
 newB[k] = b[k];
 }
 newB[length] = '.';
 newB[length + 1] = '0';
 dotIndex = index;
 b = newB;
 }
 double sum = 0;
 index = 0;
 int exponent = dotIndex;
 while(exponent != 0) {
 if(b[index] == '1'){
 sum = sum + pow (2, exponent-1);
 }
 index++;
 exponent--;
 }
 while(b[index] != '0円'){
 if(b[index] == '1'){
 sum = sum + pow (2, exponent);
 }
 exponent = exponent - 1;
 index++;
 }
 return sum;
}

I will leave it to you to comment the code properly.

Please note: The code now is a bit longer than yours, but it is able to handle more things.

---

EDIT:

After one of the comments below, I decided to also show a version without C-style arrays, and without new. This also should solve the problem with possible memory leaks. So all in all, this is the better version (I will leave the old version, because the questioner asked for a function double abc(char* b)):

#include <iostream>
#include <cmath>
#include <string>
double abc(std::string b);
int main() {
 std::cout << abc("1101.101") << "\n";
 return 0;
}
double abc(std::string b){
 if(b.compare("") == 0 || b.compare(".") == 0) { //Empty String or only "."
 return -1;
 }
 if(b[0] == '.') { //No positions before the decimal point
 b.insert(0, "0");
 }
 int countDots = 0;
 size_t index = 0;
 int dotIndex = -1;
 do{
 if ((b[index] == '1') || (b[index] == '0') || (b[index] == '.')){
 index++;
 if(b[index] == '.'){
 countDots++;
 dotIndex = index;
 }
 if(countDots == 2){
 return -1;
 }
 } 
 else {
 return -1;
 }
 } while(index < b.length());
 if(dotIndex == -1) { //No dot found, but not empty string, so you have a natural number
 b.push_back('.');
 b.push_back('0');
 dotIndex = index;
 }
 double sum = 0;
 index = 0;
 int exponent = dotIndex;
 while(exponent != 0) {
 if(b[index] == '1'){
 sum = sum + pow (2, exponent-1);
 }
 index++;
 exponent--;
 }
 while(index < b.length()){
 if(b[index] == '1'){
 sum = sum + pow (2, exponent);
 }
 exponent = exponent - 1;
 index++;
 }
 return sum;
}

• Ilkhd: if(b == "" || b == ".") { //Empty String or only "." return -1; } does not work the way you think it does
• user214772: Thank you. I use strcmp now instead.
• L. F.: Instead of promoting the use of C-style arrays and gratuitous usage of new, please make proper use of std::string and std::string_view. The comparison problem is just one demonstration of how error prone C-style arrays are. And it is very easy to leak memory with your implementation.
• user214772: xyz9 wrote in his question: "Write and test your own double abc (char * b) function", so I thought he has to do it this way. Of course std::string would be the better option. Could you give an example when memory leak happens?
• L. F.: The new version is better. In the old version, the new arrays are leaked (b = newB doesn't do what you think it does). For the new version, some points to consider: string_view; a.compare(b) == 0 => a == b; +=; not using pow for integer powers of 2; fewer manual loops if possible (or simplify the control flow); etc.

Answer 2

Code structure

This problem has three sub-parts, first Check if binary string is valid, second parse integral part and third parse fractional part. So a possible logical structure of code should be

float ConvertToDecimal(const std::string & inputBinaryString)
{
 int dotIndex;
 if (!FValidBinaryString(inputBinaryString, dotIndex))
 throw std::exception("Error: ConvertToDecimal Invalid binary string");
 int integralPart = ParseIntegralPart(inputBinaryString, dotIndex);
 float fractionalPart = ParseFractionalPart(inputBinaryString, dotIndex);
 return integralPart + fractionPart;
}

Based on this logical breaking, three functions should be implemented

bool FValidBinaryString(const std::string& input, int& dotIndex) //Out param dotIndex
int ParseIntegralPart(const std::string& input, int dotIndex)
float ParseFractionalPart(const std::string& input, int dotIndex)

Your current code also does similar steps but in monolithic way.

C++ specific comments

1. Avoid "using std namespace". One of the obvious reason is avoiding cases where same name is coming from two different namespaces and creates error and confusion. Common utility functions like min, max are defined in many namespaces. Using full name makes code more readable. If name is getting too long, use aliases.
2. Variable and function names are very very important. Be consistent in naming ans use words which convey meaning. One of the very basic rule to start can be "Functions should start with verbs and variables as noun"
3. Function which parses binary string can not modify input string. So it makes sense to pass by const reference. Passing by pointer is not good idea here. Two reasons, Callee function needs to ensure it is not de-referencing an null pointer, so a null check will be needed, second callee can inadvertently modify input string.
4. No need to use pow function in inside loop. You need to just keep doubling (or halving) the place value. So multiplication (or division) by 2 should serve the purpose.
5. Use ASCII value of '1', '0', '.' in comparison.

Answer 3

Code Review

I suggest using std:string so you can use existing std functions and for each / for instead of do while.

Separate the validation to different function.

Edge cases:

• non frictions
• empty string
• "."

For those inputs this loop

while(b[i]!='.'){ i++; }

will cause segmentation fault or infinite loop.

Alternative solution

You wrote

Is there any faster way to solve this exercise?

So I understand you ask for alternative solution.

Here is my solution, excluding the validation:

1. split the binary friction to 2 strings
2. Convert each part to decimal number.
3. Convert the numbers to strings and concat them with '.'
4. Parse the string as double

I believe this will be shorter

• shanif: I don't understand the down vote.
• rolfl: I imagine the downvote(s) are due to your answer being a suggestion for a different solution, and not a review of the OP's solution. This is Code Review, not "Solve my problem your way"
• shanif: But he ask it "Is there any faster way to solve this exercise?"
• L. F.: On Code Review, answers are expected to provide at least one observation about the code, not just suggest alternative solutions. See Why are alternative solutions not welcome? for more information.