5
\$\begingroup\$

I am trying to implement a program which can convert number into words. My code can convert numbers between 0 - 999. It uses recursive function calls and simple arithmetic operations. Can you please review it and give me your feedback?

I used three different vectors to store words such as "Six" and "Eleven" and then access there indexes according to your input.

numberToWord.h

#ifndef numberToWord_H
#define numberToWord_H

#include <vector>
#include <string>

class numbertoword
{
public:
    numbertoword();
    ~numbertoword(){};

public:
    std::vector<std::string> one2nine;
    std::vector<std::string> elevent2ninteen;
    std::vector<std::string> twoDigit;
    std::vector<std::string> threeDigit;

public:
    void initialize();
    void convert(int number);
    int countNumber(int number);
    void calculate(int number, int count);
    void display(int digit);

private:
    int xN, yN;
    int originalNumber;

};



#endif

Source.cpp

#include <iostream>
#include "numberToWord.h"

numbertoword::numbertoword() : xN(0), yN(0), originalNumber(0)
{
}

void numbertoword::initialize()
{
    //one to nine
    one2nine.push_back("Zero");
    one2nine.push_back("One");
    one2nine.push_back("Two");
    one2nine.push_back("Three");
    one2nine.push_back("Four");
    one2nine.push_back("Five");
    one2nine.push_back("Six");
    one2nine.push_back("Seven");
    one2nine.push_back("Eight");
    one2nine.push_back("Nine");

    //eleven to ninteen
    elevent2ninteen.push_back("Eleven");
    elevent2ninteen.push_back("Twelve");
    elevent2ninteen.push_back("Thirteen");
    elevent2ninteen.push_back("Fourteen");
    elevent2ninteen.push_back("Fifteen");
    elevent2ninteen.push_back("Sixteen");
    elevent2ninteen.push_back("Seventeen");
    elevent2ninteen.push_back("Eighteen");
    elevent2ninteen.push_back("Nineteen");

    //TwoDigit
    twoDigit.push_back("Ten");
    twoDigit.push_back("Twenty");
    twoDigit.push_back("Thirty");
    twoDigit.push_back("Forty");
    twoDigit.push_back("Fifty");
    twoDigit.push_back("Sixty");
    twoDigit.push_back("Seventy");
    twoDigit.push_back("Eighty");
    twoDigit.push_back("Ninety");

    //threeDigit
    threeDigit.push_back("Hundred");
}

void numbertoword::display(int digit)
{
    if( originalNumber < 10)
    {
        std::cout<< one2nine[digit]<<" ";
    }
    else if( originalNumber < 20)
    {
        std::cout<< elevent2ninteen[digit - 1]<<" ";
    }
    else if( originalNumber < 100)
    {
        std::cout<< twoDigit[digit - 1]<<" ";
    }
    else if( originalNumber < 1000)
    {
        std::cout<<one2nine[digit]<<" "<<threeDigit[0]<<" ";
    }
}

void numbertoword::calculate(int number, int count)
{
    if( number < 10)
    {
        display(number); return;
    }

    xN = number / count;    
    yN = number - (xN * count);

    if( number < 20 && number != 10)
    {
        display (yN ); return;
    }
    else if(number < 100 && yN == 0)
    {
        display( xN ); return;
    }
    else if(number < 100 )
    {
        display( xN );
    }
    else if(number < 1000 && yN == 0)
    {
        display(xN);
        return;
    }
    else if(number < 1000 )
    {
        display(xN);
    }
    originalNumber = yN;
    count = countNumber(originalNumber);
    calculate(yN, count);


}

int numbertoword::countNumber(int number)
{
    int c = 1;
    while( number > 9)
    {
        number = number / 10;
        c *= 10;
    }

    return c;
}

void numbertoword::convert(int number)
{
    initialize();

    originalNumber = number;

    int c = countNumber(number);
    calculate(number, c);

}

int main()
{
    numbertoword n2w;
    n2w.convert(999);

    std::cin.get();
    return 0;
}
\$\endgroup\$
8
\$\begingroup\$

numberToWord.h

  • Consider renaming numbertoword to NumberToWord. This uses a naming convention referred to as PascalCase, which is different from your variables and functions. Also notice that the compound words have been emphasized. This makes it easier for others to read the full name.

  • Since you're not using your own destructor, you can just leave it out. The compiler will provide one for you that should be suitable.

  • These names make no sense to me:

    int xN, yN;
    

    Unless these shortened names are obvious in the program, they should be spelled-out entirely so that others can understand them. It may also benefit you in case you ever forget what they mean.

Source.cpp

  • Elaborating on what @vnp has mentioned about initializer lists, you can use it in place of the multiple calls to push_back():

    one2nine { "one", "two", "three" /* ... */ };
    

    Moreover, consider renaming the vector to oneToNine, which is less-awkward of a name. Apply this to the other similar names as well.

    Better yet (regarding the use of vectors), you can use std::array instead. It would be better-suited for this task as you're not needing a dynamic data structure here.

  • This can be simplified:

    number = number / 10;
    

    by using the /= operator:

    number /= 10;
    
  • With the existing curly braces, these should be on separate lines:

    display(number); return;
    
  • You should be doing input-validation with this. If the user inputs a non-numerical value or a value below 0 or above 999, the program should display an appropriate error message and then terminate.

  • This program can be made more usable by accepting command line arguments:

    int main(int argc, char* argv[])
    {
        int number;
    
        // the file name is considered an argument
        if (argc > 1)
        {
            number = std::atoi(argv[1]);
        }
        // only file name given
        else if (argc == 1)
        {
            std::cin >> number;
        }
    
        // ...
    }
    

    (std::atoi() requires <cstdlib>)

  • It would make more sense to construct numbertoword objects with the original number, rather than passing it to convert(). The function has no business doing that, and this initialization should only be done by the constructor. The function should also no longer take arguments.

    numbertoword n2w(999);
    n2w.convert();
    

    In order for this to work, you should replace the default initializer list with one that takes an argument:

    numbertoword::numbertoword(int originalNumber)
        : xN(0)
        , yN(0)
        , originalNumber(originalNumber)
    {}
    

    (I've rearranged the list so that it's easier to maintain the data members.)

\$\endgroup\$
7
\$\begingroup\$

All vectors shall be private. You don't want a client to know about them, and especially you don't want a client to modify them.

Since the question is C++11 tagged, I'd recommend to use initializer_list approach to initialize them.

In any case, intitialize() belongs to a constructor.

I am not quite sure that countNumber and calculate should be exposed publicly. In any case, a public method named calculate must calculate something important. As a client of your class I only want to convert a number into its textual representation. Why do I ever want to calculate anything in the process?

\$\endgroup\$
  • \$\begingroup\$ +1 any method called initialize() or similar is a code smell imho. All classes should be constructed as close as possible to a useful state. \$\endgroup\$ – Emily L. Sep 22 '14 at 20:15

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.