Is this code efficient?

[Ch1156]

I wrote this program to test my knowledge of classes, and I think I finally got the hang of them. I want to move on to inheritance next but I would like to know if this code I wrote is efficient, mainly is there any code that can be written better? I could still do some stuff like error handling but as far as i'm concerned this code is finished, it works exactly like i want it to.

main.cpp

/*/////////////////////////
//Name Generator Program //
//6/15/2016 @ 9:51 PM    //
//Written by Chay Hawk   //
*//////////////////////////

#include <iostream>
#include <string>
#include <vector>
#include <fstream>
#include <stdlib.h>
#include <time.h>
#include "NameGeneratorClass.h"

int main()
{
    std::cout << "Random Name Generator Version 2.2 - r38\n" << std::endl;

    NameGenerator NG;

    srand(time(0));

    int choice = 0;

    while(choice != 3)
    {
        std::cout << "1. Generate Random Names" << std::endl;
        std::cout << "2. Export chosen amount of random names" << std::endl;
        std::cout << "3. Exit" << std::endl;
        std::cout << ">";
        std::cin >> choice;

        std::cin.clear();
        std::cin.ignore(500, '\n');

        switch(choice)
        {
            case 1:
                while(true)
                {
                    NG.DisplayNames();

                    std::cin.clear();
                    std::cin.ignore(500, '\n');

                    std::cin.get();
                }
                break;

            case 2:
                NG.ExportNames();
                break;

            case 3:
                return 0;
                break;

            default:
                std::cout << "Something went wrong somewhere" << std::endl;
        }
    }

    return 0;
}

NameGeneratorClass.h

#ifndef NAMEGENERATORCLASS_H_INCLUDED
#define NAMEGENERATORCLASS_H_INCLUDED

#include <algorithm>

class NameGenerator
{
    public:
        NameGenerator(int firstNameLength = 0, int lastNameLength = 0, int entireNameLength = 0,
                      int amountOfNames = 0, bool isExportingNames = false): firstNameLength(firstNameLength),
                                                                             lastNameLength(lastNameLength),
                                                                             entireNameLength(entireNameLength),
                                                                             amountOfNames(amountOfNames),
                                                                             isExportingNames(isExportingNames)
        {
            firstNameList.reserve(1);
            firstNameList.push_back("Doofus");

            lastNameList.reserve(1);
            lastNameList.push_back("Cromwell");
        }
        ~NameGenerator();

        void DisplayNames();
        void ExportNames();

    private:
        int firstNameLength;
        int lastNameLength;
        int entireNameLength;
        int amountOfNames;

        bool isExportingNames;

        std::string firstAndLastName;

        std::vector<std::string> firstNameList;
        std::vector<std::string> lastNameList;

        void ClearVectorInitValues();
        void LoadNames();
        void GenerateRandomName();
};

NameGenerator::~NameGenerator()
{

}

void NameGenerator::GenerateRandomName()
{
    if(isExportingNames == true)
    {
        for(int i = 0; i < amountOfNames; i++)
        {
            std::random_shuffle(firstNameList.begin(), firstNameList.end());
        }
        for(int i = 0; i < amountOfNames; i++)
        {
            std::random_shuffle(lastNameList.begin(), lastNameList.end());
        }
    }
    else
    {
        std::random_shuffle(firstNameList.begin(), firstNameList.end());
        std::random_shuffle(lastNameList.begin(), lastNameList.end());
    }
}

void NameGenerator::ClearVectorInitValues()
{
    firstNameList.clear();
    lastNameList.clear();
    firstAndLastName.clear();
}

void NameGenerator::LoadNames()
{
    ClearVectorInitValues();

    std::ifstream loadFirstNameList;
    loadFirstNameList.open("First Names.txt");

    std::ifstream loadLastNameList;
    loadLastNameList.open("Last Names.txt");

    std::string firstName;

    while(getline(loadFirstNameList, firstName, '\n'))
    {
        firstNameList.push_back(firstName);
    }

    std::string lastName;

    while(getline(loadLastNameList, lastName, '\n'))
    {
        lastNameList.push_back(lastName);
    }

    loadFirstNameList.close();
    loadLastNameList.close();
}

void NameGenerator::ExportNames()
{
    LoadNames();

    std::cout << "How many names do you want to export?\n" << std::endl;
    std::cin >> amountOfNames;

    isExportingNames = true;

    GenerateRandomName();

    std::ofstream exportNames;
    exportNames.open("Exported Names.txt");

    int elementNumber = 1;

    for(int i = 0; i < amountOfNames; i++)
    {
        exportNames << elementNumber++ << ". " << firstNameList[i] << " " << lastNameList[i] << std::endl;
    }
}

void NameGenerator::DisplayNames()
{
    LoadNames();
    GenerateRandomName();

    static int elementNumber = 1;

    //Since we have random shuffle, we can just output the first element and it will still be a random name.
    std::cout << elementNumber++ << ". " << firstNameList[0];
    std::cout << " ";
    std::cout << lastNameList[0];
}

#endif // NAMEGENERATORCLASS_H_INCLUDED 

[48T7M4Gy]

One aspect to consider in class design is generality and in this case it is better to avoid cout's because it immediately restricts the use of the class to console programs. It is better to use stringstreams or if you are not across them use string getters and cout those strings in main()

Another aspect is the repetition of the functions LoadNames and GenerateNames (which are also private). You might like to consider making their functionality part of the constructor with all subsequent access to the generated names being via direct use of the member vectors.

[Ch1156]

Hi, so how would the stringstream thing look in the code? I have used them before, not often though, i'm a little rusty but I do know how to write them. as for the second thing, i'm very confused by what you mean.

[jlb]

You should also be using the stream class constructors instead of using the open() member function.

   // Instead of:
   std::ofstream exportNames;
   exportNames.open("Exported Names.txt");

   // Use the constructor:
   std::ofstream exportNames("Exported Names.txt");

You should always check the that the files open properly before you try to use the streams.

What are the purposes of all those "length" variables? A vector knows it size(), and a string knows it's length() why not just use those values?

[48T7M4Gy]

This article gives a quick intro into how streams operate. The code looks complicated but the idea is simple as it just provides a substitute to cout << and cin >> instead of std (console) output and input << and >> goes to a string stream which can be used anywhere with much greater flexibility! e.g. if you were programming in Windows environment.

http://www.cplusplus.com/forum/beginner/29402/

[48T7M4Gy]

As for "the second thing" effectively what I suggest is putting the code for reading in the file, generating the list and loading the member vectors once and for all in the code for the constructor.

Everywhere else in the class methods use the member vectors.

However, closer inspection of your class and how main interacts with it reveals to me that LoadNames method is unusual in that it mixes the function, which is to add a name to the list with the console interface. This is probably a major change/simplification you don't want to know about but a better way is to have a addName( Xyz xyz) method get xyz in main().

The principle with all of this is the class is an abstract data type not the user interface. Each object just enables the vectors to be added to, read and altered but not much else. :)

[Ch1156]

I cannot seem to get the stringstream thing working in my class, I tried doing it the way they had it in the post, applying the corrections as well, however it did not work.

[dhayden]

NameGenerator.h assumes that you have already included fstream and iostream. A header file should include whatever it needs.

Code in the header file should be declared as inline. Otherwise if you include the header file from multiple compilation units, you'll get multiple definitions of that code.

firstAndLastName is unused.

In GenerateRandomName(): why do you randomly shuffle the name lists multiple times? And why do you care if isExportingNames is set? Are here we call that a "computed come-from" because the code has to know where it's being called from. And why is it called GenerateRandomName when it doesn't actually generate names at all? This should be called shuffleNames() and it should just shuffle the names.

Factor out common code in LoadNames. By doing this you can do away from ClearVectorInitValues():

static void loadFromFile(const char *name, std::vector<std::string> &vec)
{
    vec.clear();
    std::ifstream theFile(name);

    std::string str;

    while (getline(theFile, str, '\n')) {
        vec.push_back(str);
    }
    theFile.close();
}

void
NameGenerator::LoadNames()
{
    loadFromFile("First Names.txt", firstNameList);
    loadFromFile("Last Names.txt", lastNameList);
}

A function should do only one thing. ExportNames() should export whatever names are currently in the collection. It should not call LoadNames().

ExportNames should take a stream parameter. All the code that asks about the file name should be in the caller.

DisplayNames() is poorly named since it only displays one name.

main() line 39: how do you ever exit this loop?
main() line 59: Something went wrong? What went wrong? What can the user do to correct it? Try to get in the habit of reporting exactly what went wrong and what can be done.

The code could be made more efficient by reading the names just once.

Here is an alternative. This is all in one file, but it's easy enough to split it out.

#ifndef NAMEGENERATORCLASS_H_INCLUDED
#define NAMEGENERATORCLASS_H_INCLUDED

#include <fstream>
#include <iostream>
#include <algorithm>

class NameGenerator
{
  public:
  NameGenerator()
    {
        LoadNames();
    }
     ~NameGenerator();

    void shuffle();
    void Display(std::ostream &);

  private:
    std::vector < std::string > firstNameList;
    std::vector < std::string > lastNameList;

    void LoadNames();
};

NameGenerator::~NameGenerator()
{

}

void
NameGenerator::shuffle()
{
    std::random_shuffle(firstNameList.begin(), firstNameList.end());
    std::random_shuffle(lastNameList.begin(), lastNameList.end());
}

static void loadFromFile(const char *name, std::vector<std::string> &vec)
{
    vec.clear();
    std::ifstream theFile(name);

    std::string str;

    while (getline(theFile, str, '\n')) {
        vec.push_back(str);
    }
    theFile.close();
}

void
NameGenerator::LoadNames()
{
    loadFromFile("First Names.txt", firstNameList);
    loadFromFile("Last Names.txt", lastNameList);
}

void
NameGenerator::Display(std::ostream &os)
{
    for (unsigned i = 0; i < firstNameList.size(); ++i) {
        os << i+1 << ". " << firstNameList[i]
           << " "
           << lastNameList[i]
           << '\n';
    }
}

#endif // NAMEGENERATORCLASS_H_INCLUDED

/*/////////////////////////
//Name Generator Program //
//6/15/2016 @ 9:51 PM    //
  //Written by Chay Hawk   //
*//////////////////////////

#include <string>
#include <vector>
#include <stdlib.h>
#include <time.h>
// #include "NameGeneratorClass.h"

int
main()
{
    std::cout << "Random Name Generator Version 2.2 - r38\n" << std::endl;

    NameGenerator NG;

    srand(time(0));

    int choice = 0;

    while (choice != 3) {
        std::cout << "1. Randomize Names" << std::endl;
        std::cout << "2. Display names" << std::endl;
        std::cout << "3. Export names" << std::endl;
        std::cout << "4. Exit" << std::endl;
        std::cout << ">";
        std::cin >> choice;

        std::cin.clear();
        std::cin.ignore(500, '\n');

        switch (choice) {
        case 1:
            NG.shuffle();
            break;

        case 2:
            NG.Display(std::cout);
            break;
        case 3:
            {
                std::ofstream exportNames("Exported Names.txt");
                NG.Display(exportNames);
                exportNames.close();
            }
            break;

        case 4:
            return 0;
            break;

        default:
            std::cout << choice << "is not a valid choice" << std::endl;
        }
    }

    return 0;
}