92 Spoons AI, sort of an AI in C++

Question

I have made an AI that takes a user input and returns a predefined value. It that stores its info in a file (with the name of the file "default.92ai"). I thought that it didn't seem very efficient, considering it was a project that helped me learn C++.

I also think I might have added a bunch of really stupid code when I was debugging and forgot to take it out. I split the project into two files to make a sort of API.

I have an extra file that I recently created to test this "API". This is a separate executable, and is just to test the API. (So I do gcc -o ai ai.cpp and separately do gcc -o test test.cpp)

If you have any suggestions, I would be happy to take them, except using namespace std edits. I am Edit: was (I am scared of what would happen to me if I keep using using namespace std) too lazy to type std::.

GitHub repo

ai.cpp

#include <iostream>
#include <fstream>
#include <vector>
#include <sstream>
#include <algorithm>
#include <thread>
#include <unistd.h>
#include "files.h"
#ifdef __MINGW32__
#include <mingw.thread.h>
#endif
#ifdef WIN32
#include <windows.h>
#else
#include <unistd.h>
#endif // win32
using namespace std;
void sleepcp(int milliseconds){
#ifdef WIN32
    Sleep(milliseconds);
#else
    usleep(milliseconds * 1000);
#endif // win32
}
    void load(/*int size*/){
        float progress = -0.01;
        int time = 100;
        /*if (size<100){
            time = size*0.5;
        }
        if (size<1000&&size>100){
            time = size*0.05;
        }
        if (size>1000){
            time = size*0.01;
        }*/
        while (progress < 1.0) {
            progress += 0.01;
            int barWidth = 70;

            cout << "[";
            int pos = barWidth * progress;
            for (int i = 0; i < barWidth; ++i) {
                if (i < pos) cout << "=";
                else if (i == pos) cout << ">";
                else cout << " ";
            }
            cout << "] " << int(progress * 100.0) << " %\r";
            cout.flush();
            sleepcp(time);
        }
        cout << endl;

    }
int main () {
    //load default conf
    files conf;
    conf.filename="default.92ai";
    //conf.checksize();
    thread loadbar (load/*,conf.size*/);
    conf.read();
    loadbar.join();
    cout << "Welcome to the 92 Spoons AI interface!"<<endl<<"If you need a tour around, say 'tour'."<<endl;
    while(true){
        string q;
        //allows for spaces in commands
        getline(cin,q);
        if (q=="exit"){
            return 0;
        }else{
            if (q=="reload"){
                conf.reload();
                }
        else{
                if (q=="load"){
                    //not tested
                    cout<<"What file shall I load?"<<endl;
                    string file2load;
                    cin>>file2load;
                    files file2;
                    file2.filename=file2load;
                    }
            else{
                    if (q=="import"){
                        cout<<"What file shall I import?"<<endl;
                        string file2load;
                        cin>>file2load;
                        fstream file2;
                        file2.open(file2load);
                        string line;
                        while(getline(file2,line)){
                            conf.add(line);
                            //lets hope this works i.e. not tested
                        }
                    }else{
        if (q=="write"){
            string edit;
            getline(cin,edit);
            conf.add(edit);
        }else{
            if (q=="tour"){
                cout<<"Hi!"<<endl<<"You can type something and this AI will respond!"<<endl<<"If you want to make a new command, type write."<<endl<<"Then type what you expect a user to type, like llama."<<endl<<"Then, type a colon."<<endl<<"Finally, tell me what I should say to respond."<<endl<<"For example, llama:No, llama, no!"<<endl;
            }else{
                vector<string>::iterator it;
                it = find (conf.first.begin(), conf.first.end(), q);
                if (it != conf.first.end()){
                    //TODO: Fix this conversion error
                    long nPosition = distance (conf.first.begin (), it);
                    cout << conf.second.at(nPosition)<<endl;
                }else{
                    cout << "Not found!" <<endl;
            }
        }
    }
    }
    }
        }
}
    }
        return 0;
}

files.h

#ifndef UPDATE_H
#define UPDATE_H
#include <iostream>
#include <string>
#include <vector>
#include <fstream>
#include <sstream>
using namespace std;
class files{
    string edit;
public:
    int size;
    vector<string> first;
    vector<string> second;
    fstream file;
    string filename;
    void add(string edit);
    void keepline(string edit);
    void fileopen();
    void read();
    void close();
    void reload();
    void start();
    void end();
    int checksize();
    void load(string loadname);
};
int files::checksize(){
    fstream filesize;
    filesize.open(filename,ios::binary);
    streampos begin,end;
    begin = filesize.tellg();
    filesize.seekg (0, ios::end);
    end = filesize.tellg();
    int size = end-begin;
    //if you hate warnings
    /**psize++;
    *psize--;*/
    files::start();
    return size;
}
void files::start(){
    fstream *pfile;
    pfile = &file;
    (*pfile).seekg (0, ios::beg);
}
void files::end(){
    fstream *pfile;
    pfile = &file;
    (*pfile).seekg (0, ios::end);
}
void files::fileopen(){
    fstream *pfile;
    pfile = &file;
    (*pfile).open(filename);
}
void files::add(string edit){ //for experienced users
    fstream filewrite;
    filewrite.open(filename);
    filewrite.seekg(0,ios::end);
    filewrite << edit <<endl;
    filewrite.close();
}
void files::keepline (string edit){
    fstream filewrite;
    filewrite.open(filename);
    filewrite << edit;
    filewrite.close();
}
void files::read(){
    files::fileopen();
    fstream *pfile;
    pfile = &file;
    vector<string> *pfirst;
    pfirst = &first;
    vector<string> *psecond;
    psecond = &second;
    string line;
    string header;
    while (getline(*pfile,line)){
        stringstream split (line);
        string one;
        string two;
        getline(split,one,':');
        (*pfirst).push_back(one);
        getline(split,two);
        (*psecond).push_back(two);
    }
    if (first.at(0)!="92spoons AI header"){
        cout<<"Oh no! A file was loaded that does not have a correct header. Make sure you loaded a 92ai file, and that you didn't make a typo on the header."<<endl;
        //cout<<"|"<<header<<"|";
        exit(1);
    }
}
void files::close(){
    fstream *pfile;
    pfile = &file;
    (*pfile).close();
}
void files::reload(){
    vector<string> *pfirst;
    pfirst = &first;
    vector<string> *psecond;
    psecond = &second;
    files::close();
    (*pfirst).clear();
    (*psecond).clear();
    files::read();
}
void files::load(string loadname){
    files file2;
    file2.filename=loadname;
    string line;
    string header;
    getline(file2.file,header);
    if (header!="92spoons AI header"){
        cout<<"Oh no! A file was loaded that does not have a correct header. Make sure you loaded a 92ai file, and that you didn't make a typo on the header."<<endl;
        exit(1);
    }
    while (getline(file2.file,line)){
        stringstream split (line);
        string one;
        string two;
        getline(split,one,':');
        first.push_back(one);
        getline(split,two);
        second.push_back(two);
}
}
#endif

test.cpp

#include <iostream>
#include <fstream>
#include <vector>
#include <sstream>
#include <algorithm>
#include <thread>
#include <unistd.h>
#include "files.h"
#ifdef __MINGW32__
#include <mingw.thread.h>
#endif
#ifdef WIN32
#include <windows.h>
#else
#include <unistd.h>
#endif // win32
#ifdef NODEBUG
bool debug = false;
#else
bool debug = true;
#endif
using namespace std;
int main(){
    files test;
    test.filename="default.92ai";
    if(test.filename!="default.92ai"){
        if(debug){
            cout<<"Var storage test failed! Here is what the var was instead:"<<test.filename<<endl;
        }
        return -1;
    }
    if(debug){
        cout<<"Var storage test passed!"<<endl;
    }
    test.fileopen();
    if(test.file.bad()){
        cout<<"File opening test failed!"<<endl;
        return -1;
    }
    test.close();
    if(debug){
        cout<<"File opening test passed!"<<endl;
    }
    //add read debug
    test.fileopen();
    string testread;
    getline(test.file,testread);
    if (testread!="92spoons AI header"){
        cout<<"File reading test failed!"<<endl;
        return -1;
    }
    cout<<"File reading test passed!"<<endl;
    srand(time(NULL));
    string name = "test"+to_string(rand());
    test.add((name+":test").c_str());
    test.close();
    test.read();
    vector<string>::iterator it;
    it = find (test.first.begin(), test.first.end(), name);
    if (it == test.first.end()){
        cout<<"File writing test failed!"<<endl;
        return -1;
    }else{
        //TODO: Fix this conversion error
        long nPosition = distance (test.first.begin (), it);
        if (debug){
            cout << "File writing test passed!"<<endl;
        }
    }
}

Disclosure: 92 Spoons AI's name is based off of 92spoons.com, my friend's website. Now I'm not breaking the rules!

Answer 1

There's a lot to say here, so let's not waste any time and jump right into it:

1. You said that you didn't want to hear the using namespace std;-drill, but using namespace std; is so dangerous that I'm just going to ignore that and give it to you anyways. So here it goes: Don't use using namespace std;, you're going to shoot yourself in the foot eventually. Your laziness is not an excuse to introduce subtle bugs into your program that will make your bits rot and more and more people pull their hair out while trying to compile your code as time goes on. Most importantly, never do that in a header. People are really, really, really going to hate you.
2. Organize your includes. First of all, includes should be in alphabetical order to facilitate a check of whether all required includes are there. Secondly, it is preferred to group your includes (and separate those groupings with a blank line) by what part of an environment they belong to. Commonly, people start by including the header a particular file implements (if any), then headers from the same project you're working on, then headers from different projects and libraries and finally standard headers. This has the added benefit of checking for missing includes: If you forgot to include any header in one of the files under your immediate control, the compiler will issue an error whereas that missing include might be hidden by previous includes if this order is not kept.
3. You actually have what I would call an include error: Both in ai.cpp and test.cpp, you include unistd.h and then include it again if __WIN32__ is not defined. Likely you'd only want to include it if you're not working on a Windows platform, so you should remove the general include.
4. void sleepcp(int milliseconds) raises an eyebrow. Why do you use int here? What happens if I sleep for -5 milliseconds? That parameter milliseconds should be unsigned, because signed values do not make much sense in your case (and will be promoted to their respective unsigned type in the function call in the body anyway).
5. Don't use std::endl. It's horrible. It does not only what its name promises (i.e. print a newline character), but also flushes the underlying output buffer which can turn out to be a big performance killer. Just use '\n' instead.
6. cout << "["; is better written as cout << '['; because the latter passes the value directly to operator<< whereas the first passes a pointer to string literal which has to be dereferenced (and most likely also entails a call to std::strlen).
7. Kind of reiterating on point 4, but why do you think barWidth in load() should be of type int? Can barWidth ever be negative? Doesn't seem to be the case, so I recommend unsigned or related. Think about your type usage more!

8. Please fix your indentation and whitespace usage.

       }
       }
       }
           }
   }
       }
   

   is just completely unacceptable (see main() in ai.cpp). It suggests that you have not figured out when to indent by how much at all, so please think about that. Also, please be consistent about where you add a whitespace or leave one out, for example before the parentheses of a function call(there's no "like this"-answer here, as coding style varies greatly from programmer to programmer. However, indentation always serves the goal of readability, so you should stick with whatever seems the most readable to you. As a rule of thumb, more whitespace is better than less, so when in doubt, add a whitespace (although not overdoing this is important, too)).

9. The inner body of the while (true) loop in main() in ai.cpp would benefit from a switch-case statement instead of all these if-else blocks.

10. Why do you have comments containing code sprinkled throughout your program? I get the impression that what you've submitted here is some non-finished, hacky code that you deemed somehow good enough to present to us. However, this makes code review only so much efficient. Before submitting anything here, you should clean up your code and make it as good as you possibly can. Otherwise, we are just going to write hints about things you already knew but were too lazy or sloppy to do yet.

11. //lets hope this works i.e. not tested implies that your code does not work. Although quite radical, the only sane thing you can say is that only the code works for which you have a proof that it works. Mostly, this proof is obtained just by running your program, although you're sure to miss out on most of the edge cases (i.e. you've proven your code to work for a subsection of all available inputs). For those edge cases, we write tests. If you really are in need of a complete proof, there's fuzzing over the complete set of inputs (which is unlikely to end before the heat death of the universe in most cases) or even a mathematical proof of your algorithm's working (but for everyday applications those are hardly ever required). Still, this means that you have uploaded what is basically broken code, since having not tested it even once is the same as saying you have no idea whether it works correctly or not, and having no idea if the code will even run is, in a conservative sense, the same as saying it won't.

12. You seem to, somewhat at least, have missed the purpose of object oriented programming. Your class file is not much different from a C struct: Neither do you seem to care much for encapsulation or the single responsibility principle, nor do you work with any of the provided special member functions such as constructors, destructor etc. I don't really want to go into detail here because there is just so much that has to be redone, but you should look for a good book on OOP and class design in C++ and start from there.

13. The concept of references in C++ seems to have eluded you somewhat, since you always seem to use pointers instead (for example in most of the member functions of file). Please read up on this topic, as references belong to the core features C++ offers.

14. Still, when using pointers, prefer obj->func() to (*obj).func() as this is the dedicated syntax and makes your code less confusing.

---

In the end, your code leaves me somewhat confused. Although you even use the preprocessor to organize your includes correctly, and use standard algorithms where appropriate, a lot of your code seems to tell me that you're still very much a beginner in writing C++.

I think the best thing to do for you right now is to pick up a good book and read, about C++ and about OOP, and then apply the things that you learn to your code.

Answer 2

Here are some things that may help you improve your code.

Don't abuse using namespace std

I know you said you don't want to hear this, but your code is unusable as a library until this is addressed. Putting using namespace std at the top of every program is a bad habit that you'd do well to avoid. If you put it into a header (a .h file) it makes your library unusable because it means that the not only will your own functions, classes, variable, etc. be put into the global namespace, but so will every include file after yours, meaning that the compiler will generate different code dependent on the order of the #include files. That's fatally broken and must be addressed if anyone is to use your API.

Use consistent formatting

Using consistent formatting helps readers of your code understand it without distraction. This code is mostly well formatted, but tail end of main in ai.cpp in particular is inconsistent and hard to read.

Remove commented-out code

Leaving in code that has been commented out is distracting and unhelpful. It conveys the sense that you haven't spent much time cleaning up the code before presenting it for review which is not likely to be useful in getting good reviews. If the code is not worth your time, why should I spend my time on it? Generally, it's good to make one last pass, getting the code as good as you can make it before presenting it for review by others, whether here or if you ever write code professionally and get it reviewed by colleagues.

Separate interface from implementation

The interface is the part in the files.h file and the implementation should be in files.cpp file. The reason is that you might have multiple source files including the .h file but only one instance of the corresponding .cpp file. In other words, split your existing files.h file into a .h file and a .cpp file.

Make sure you have only required #includes

Users of this code should be able to read and understand everything they need from the implementation file. That means that only #includes essential to being able to understand the interface should be in the .h file. In this case, only <string>, <vector> and <fstream> are required for the .h file, but the implementation (.cpp file) additionally needs <iostream> and <sstream>.

Don't use std::endl if '\n' will do

Using std::endl emits a \n and flushes the stream. Unless you really need the stream flushed, you can improve the performance of the code by simply emitting '\n' instead of using the potentially more computationally costly std::endl.

Simplify functions by understanding the standard

The current version of files::checksize() is this:

int files::checksize(){
    std::fstream filesize;
    filesize.open(filename,std::ios::binary);
    std::streampos begin,end;
    begin = filesize.tellg();
    filesize.seekg (0, std::ios::end);
    end = filesize.tellg();
    int size = end-begin;
    //if you hate warnings
    /**psize++;
    *psize--;*/
    files::start();
    return size;
}

The code simply doesn't need to be that long. If you happen to have a C++17 compliant compiler, you could simply use std::filesystem::file_size(). Alternatively, with any C++11 compiler, you could use this:

int files::checksize(){
    std::fstream infile{filename, std::ios::ate | std::ios::binary};
    return infile.tellg();
}

The std::ios::ate flags says to seek to the end of the file when opening and tellg() returns the length because the pointer is already at the end of the file. Also note that because infile is locally declared, it will automatically be closed when the function ends and infile goes out of scope. (The default destructor closes the file, if it's open.) This same kind of simplification can be done almost everywhere in files.cpp to reduce and simplify the code.

Understand name scopes

This bit of code in response to a load command does not do what you think it does:

if (q=="load") {
    //not tested
    std::cout<<"What file shall I load?"<<"\n";
    std::string file2load;
    std::cin>>file2load;
    files file2;
    file2.filename=file2load;
}

The reason is that the file2 variable is declared and then goes out of scope within the {} and is destroyed. This code won't do anything useful.

There's much more, but all I have time for at the moment.

Answer 3

Header files

You have a lot of implementation code in that header file (files.h). Header files should only contain the function or class headers, the implementation should go into an extra cpp file. That makes your code more organized and also more performant, because your code would be included as is in every file that includes your header file.

Out-commented code

Your ai.cpp contains a lot of out-commented code. When that code is not used any more, you should delete it instead of commenting, as you might later not know why you commented it out, and whether you can safely delete it or not. If you want to make sure not to use it in case it turns out that you do need it again, that should be covered by your version control system.

except using namespace std edits (I am too lazy to type std::)

Still had to add it. Rather add these five characters to every usage, enjoy your IDE's code completion and prevent name mangling problems when you import hundreds of very common names, that could easily be used by other libraries or your own code.

Coding style

Your indentation is quite off in multiple parts of your code. For example:

            }
        }
    }
    }
    }
        }
}

That makes it very difficult to see where a block starts and where it ends.

Another thing is your usage of newlines and spaces. I recommend having a look at a popular c++ style guide, like for example Google's.

Independently from a specific style guide, it is common practice to format this:

if(debug){
    cout<<"Var storage test passed!"<<endl;
}

like this:

if (debug) {
    std::cout << "Var storage test passed!" << std::endl;
}

with spaces after keywords like if and while, before opening braces (when putting them on the same line as the if condition or method header), and around operators.

When using if-else branching, it is good practice to use braces even for single lines, or at least use new lines and indentation without braces to make the branches clear to the reader. For example instead of this:

if (i < pos) cout << "=";
else if (i == pos) cout << ">";
else cout << " ";

you should rather write this:

if (i < pos)
    std::cout << "=";
else if (i == pos)
    std::cout << ">";
else
    std::cout << " ";

or preferably:

if (i < pos) {
    std::cout << "=";
}
else if (i == pos) {
    std::cout << ">";
}
else {
    std::cout << " ";
}

Performance

I will give you some hints on what to look for when coding for performance, however I don't think that performance is your biggest issue here. Until you find a bottleneck by profiling different parts of your code, I suggest you to try to improve the code's quality in general first, and it will probably be easier to find possible performance issues then.

If I understand your load() function in ai.cpp correctily, it doesn't really do anything except drawing a progress bar and artificially slowing down execution in order for the bar to not instantly be full. If the only purpose of the progress bar is to make it more fancy, but it doesn't really show the progress, I'd much rather just leave it out, or replace it with something like "Loading ..." and "Loading done".

When you have if-else branches where you can estimate the relative probability that one condition is true, that condition should go first. For example:

if (i < pos) cout << "=";
// The following is least likely to be true, so it should go last.
else if (i == pos) cout << ">";
else cout << " ";

That way, you can "short circuit" the evaluation of the conditions, because if the first one (or one of the first ones) is true, the following ones will not have to be evaluated, and less jumps in your code will occur.