what is wrong here? Unit converter

Question

it keeps on going into a continues loop not sure why!! this is not an assignment, just Practice, i'm trying to learn, I want to make a unit converter but im not sure if this is the beast way, if you have a better idea please feel free.

/// Write a program to promt for units
//calcultes and converts units

#include <iostream>
#include <string>
#include <cmath>

using namespace std;int main () {
    string re_Run, units;
    do{
    int f1;


    cout<<"enter force: >";
    cin>>f1;

    cout<<"enter units: >";
    cin>>units;

    string units, N, kN, lb, kip;
    double conv_lb, conv_N, conv_kN;

    do{
        if (f1<1000 && units == "N"){
            cout<<f1<<" N";
        }
        else if (f1>1000 && units == "kN"){
            cout<<f1<<" kN";
        }
        else if (f1>=1000 && units == "N") {//|| x== kN)
            conv_N=f1/1000;
            cout<<conv_N<<" kN"; //convert from N to kN
        }
        else if (f1<1000 && units== "lb" ){
            cout<<f1<<" lb";
        }
        else if (f1>1000 && units== "lb" ){//|| x==kip
            conv_lb=f1/1000;
            cout<<conv_lb<<" kip";
        }
        else if (f1>1000 && units== "kip" ){
            cout<<f1<<" kip";
        }
        else {
            cout<< "please enter (lb/kip/N/kN)\n    >";
            cin>>units;
        }
    }while (units == "N" || units == "kN" || units == "lb" || units =="kip");//(units != "N" && units != "kN" && units != "lb" && units !="kip");

    cout<<"re-run?";
    cin>> re_Run;
    }
    while (re_Run == "yes");
return 0;
}

Such small programs are easily debugged with a debugger. Your favorite IDE will help you. — DeiDei, Jul 05 '19 at 10:15
@Sean - it's a do-while statement. In any event, the problem is shadowing of variables - having two variables of the same name within a nested scope, so - at various points - the loop is testing old input. — Peter, Jul 05 '19 at 10:20
there are two "units" string variables. After first successful input of values, it continues to loop forever, cause you didn't reset units and always printing same conversion — Boki, Jul 05 '19 at 10:27

marcinj · Answer 1 · 2019-07-05T10:25:33.187

2

You redefine units variable which makes it empty:

cin>>units;  // here you read it

string units, N, kN, lb, kip;  // here redefinition

probably this second units is not needed

And the inner while loops because thats how the condition is set up: while (units == "N" || units == "kN" || units == "lb" || units =="kip"); if units are N then it loops.

edited Jul 05 '19 at 10:25

answered Jul 05 '19 at 10:18

marcinj

48,511
9
79
100

And one more thing. Units is only modified in the final else statement while if one of the if else statement passes the units will not be modified and the loop will continue. You probably want to put the body of else statement outside. – Anže Jul 05 '19 at 10:24
1

@Anže agree, the final `else` in the inner `while` should be removed in order to allow `std::cin >>` be executed on each iteration. – marcinj Jul 05 '19 at 10:28

Boki · Answer 2 · 2019-07-05T12:25:56.183

It can be made other way of course, but here is some solution related to yours, you can see what's wrong if analize changes

#include <iostream>
#include <string>
//#include <cmath>

using namespace std;
void printResult(const double f1, std::string& units, bool& legal) {
    cout<<f1<<units<<"\n";
    units = "";
    legal = false;
}

bool check(const std::string& units) {
    return (units == "N")||(units == "kN")||(units == "lb")||(units == "kip");
}

int main () {

string re_Run, units;
bool legal(false);
double conv(0.);
double f1(0.);

do{
    re_Run = "";
    if (f1<1000 && units == "N") {

        printResult(f1,units,legal);
    }
    else if (f1>1000 && units == "kN") {

        printResult(f1,units,legal);
    }
    else if (f1>=1000 && units == "N") {//|| x== kN)
        conv=f1/1000;
        //cout<<conv_N<<; //convert from N to kN
        units = " kN";
        printResult(conv,units,legal);
    }
    else if (f1<1000 && units== "lb" ) {
        //cout<<f1<<" lb";
        printResult(f1,units,legal);
    }
    else if (f1>1000 && units== "lb" ) {//|| x==kip
        conv=f1/1000;
        //cout<<conv_lb<<" kip";
        units = " kip";
        printResult(conv, units,legal);
    }
    else if (f1>1000 && units== "kip" ) {
        //cout<<f1<<" kip";
        printResult(f1,units,legal);
    }
    else if (legal) {
        printResult(f1,units,legal);
    }
    else {
        cout<<"enter force: >";
        cin>>f1;
        cout<< "please enter (lb/kip/N/kN)\n    >";
        cin>>units;
        legal = check(units);
        re_Run = "yes";
    }
    if (false == legal) {
        cout<<"re-run?";
        cin>> re_Run;
    }
}while (re_Run == "yes");

return 0;
}

We can see here, at least,

We dont need two nested loops

Only one units variable

Reset units and re_Run after use

Print values for legal input even if your if checking not catch use case

Variables with block scope need to be initialized if default initialization leave it uninitialized

Repeating code is move to separate functions

We dont need all those variables

etc

score -2 · Answer 3 · answered Jul 05 '19 at 10:57

-2

I think you can remove the inner "do...while" statement and just keep the if else checks. The program would still be the same without your infinite looping problem.

answered Jul 05 '19 at 10:57

Bharath Suresh

483
2
18

Thats just one problem of many, doesnt expose whats wrong with code. By my opinion, there are more important "wrongs" here, need to be noted with educational purpose – Boki Jul 05 '19 at 12:24

what is wrong here? Unit converter

3 Answers3