Console command tool

Question

I have created a little console tool that simply does commands that you enter.

The commands are stored in a txt file. Right now I only got 2 commands: getTime and getCommands.

Pretty simple, but I'm not sure I'm doing anything the most optimal way, which is why I am asking here. I was thinking about creating a whole class to get the time but I wasn't sure that would make any sense (as it seems SYSTEMTIME is already a class).

Also, I was thinking about the next command I would add is something where you can input "open 'file' ", where file is the program you want to open. I googled a bit and found a function called filefindnext and filefindfirst (to search for the file), but I'm not sure if this is the best way to do it? (I was also thinking about just adding a .txt file with the dir to the predefined files)

I hope you can help clear up my code and in general make my code better. I'm still fairly new to programming, so all constructive feedback is appreciated.

main.cpp (just a little test for my command class)

int main()
{
    command list;
    list.setCommandList();
    list.printCommands();
    while (1)
    {list.getCommand();
    }
}

commands.h

#include <iostream>
#include <fstream>
#include <string>

using namespace std;
class command
{
public:
    command();
    ~command();
    void printCommands();
    string verifyCommand(string);
    void getCommand();
    void executeCommand(string);
    void setCommandList();
private:
    std::string userinput_;
    std::ifstream file;
    string *commandlist;
    int cNr; //command number
};

commands.cpp

#include "commands.h"
#include "Time.h"
command::command(){
file.open("commands.txt");
cNr = 0;
}
command::~command()
{
    file.close();
    delete[] commandlist;
}
void command::printCommands()
{
    string line;
    cout <<"Command list:\n\n";

    for (int i = 0;i<cNr;i++)
    {
        cout <<commandlist[i] <<endl;
    }
    cout <<endl;
}
void command::setCommandList()
{
    int nrOfLines = 0;
    string cline;
    string line;
    while (getline(file,cline))
{
    nrOfLines++;
}
    file.clear();
    file.seekg(0, file.beg);
    commandlist = new string[nrOfLines];
    while (getline(file,line))
    {
        commandlist[cNr] = line;
        cNr++;
    }
}
string command::verifyCommand(string command)
{
    string line;
for (int i = 0;i<cNr;i++)
{
    if (command==commandlist[i])
        return command;
}
    return "";
}

void command::getCommand()
{
    string userinput;
    cout <<"Enter command:"<<endl;
    cin >> userinput;
    userinput_ = verifyCommand(userinput);
    if (userinput_ == "")
    {cout <<"Error, can´t find that command. Please try again"<<endl;
    getCommand();
    }
    else
    executeCommand(userinput_);
}

void command::executeCommand(string com)
{
    if (com == "getTime")
    {
        cout << string( 100, '\n' ); //clear screen
        SYSTEMTIME t;
        GetLocalTime(&t);
        cout<< "H:"<<t.wHour<<" M:"<<t.wMinute<<" S:"<<t.wSecond<<endl;
    }
    if (com == "getCommands")
    {
        cout << string( 100, '\n' );
        printCommands();
    }
}

Answer 1

• While it is okay to put using namespace std in a local scope (such as a function), it's especially problematic to put it into a header file. By doing this, anyone including it will be forced to have that namespace code exposed globally, and you shouldn't force that on anyone. Although the std namespace is most commonly used, this applies to any of them.

• You should be opening the file and doing user input in main(), not within the class. You should be receiving data from a client (such as main()) and constructing an object with this data.

  Move all of getCommand()'s functionality into main() and have that in your while(1) loop. Here, you'll get the user's command and supply it to the class until the loop ends.

  Another misleading thing here: the name getX() usually denotes a public "getter" function, which merely returns a data member to the client. It's best to use this name for a function that only does this, which getCommand() does not. As mentioned, have this functionality in main(), not within the class. You should then no longer need this function.

• Consider allowing the user to provide their own file name:

  std::cout << "Filename: ";
  std::string filename;
  std::getline(std::cin, filename);
  
  std::ifstream file;
  file.open(filename.c_str());
  

  You should have all of this in main() and no longer have the class manage the creation, opening, and closing of the file. Let the user decide how they want to manage their file. You'll also be able to terminate from main() right away if the file cannot be opened.

• I'd recommend against having file.close(); in the destructor. close() is generally unneeded as the file can close on its own, and might not work as intended here. The destructor, in general, should primarily deallocate memory when needed.

• I'm not sure why commandlist needs to be a std::string*. This could be problematic as you're having to manage the pointer and its memory yourself, and there are better alternatives to holding this data. In general, raw pointers should be used sparingly in C++.

  In setCommandList(), you're allocating an array of std::strings to that member. Why not have an std::vector of std::strings? This will do all the memory management for you, so it's much safer. You will also have access to its class functions, which could be useful.

  std::vector<std::string> commandList;
  

  This should be in place of the std::string* member, and then you can size it to nrOfLines via resize() when needed. With this, you'll no longer need to new memory nor delete it in the destructor. You won't even need to define your destructor anymore.

• Error messages, such as the one outputted in getCommand(), should be put into std::cerr instead of std::cout. The latter should just be used for general output.

• The abbreviation and comment are pointless:

  int cNr; //command number
  

  Not only is it a very confusing abbreviation, but you cannot expect readers to see this first and then remember it throughout the code. Just rename it to commandNumber (not very long of a name) and remove the comment.

• A member function that does not modify any of its data members should be const. This also protects against any accidental modifications and communicates to the reader that this function only performs read-only actions.

  In addition, any function that receives an argument of a user-defined type (such as std::string) and is not supposed to modify it should be passed by const&. This also prevents any accidental modifications and avoids a needless copy.

  These two points apply to verifyCommand(), which is not making any modifications:

  verifyCommand(std::string const& command) const {}
  

• For printCommands():

  1. It should be const as it does not modify any data members (see the above point):

     printCommands() const {}
     

  2. Leave out the label output and the extra newline. The client shouldn't have to be forced to use them if they don't want them. Just have them done around the function call.