I wrote this class which reads an SSH config file and checks whether an IP exists. If the IP does not exist, the class adds a block for that IP address.Header file:class SSHConfigFile : public QObject{    Q_OBJECTpublic:    explicit SSHConfigFile(QObject *parent = 0);signals:public slots:    void registerIP(QString ip);private:    void read();    bool find(QString ip);    void addIP(QString ip);    void write();private:    // the file name with path of config file    QString _sshConfigFileName;    QFile file;    // the actual file data    QString fileData;    // the same file data but split into lines    QStringList fileLines;};Implementation file:SSHConfigFile::SSHConfigFile(QObject *parent) : QObject(parent),    _sshConfigFileName( QStandardPaths::writableLocation( QStandardPaths::HomeLocation ) + / + .ssh/config),    file( _sshConfigFileName ){}void SSHConfigFile::read(){   if ( !file.open( QFile::ReadWrite | QFile::Text) )   {       qDebug() << Could not open file for writing;       return;   }   // read all data   QTextStream in(&file);   fileData = in.readAll();   qDebug() << fileData;   fileLines = fileData.split(QRegExp([\r\n]),QString::SkipEmptyParts);}// returns true if ip was found in config file otherwise falsebool SSHConfigFile::find(QString ip){    read();    foreach(QString  str, fileLines )    {        str = str.trimmed();        //qDebug() << str;        if (str.contains( Host, Qt::CaseInsensitive))        {            str.remove(Host, Qt::CaseInsensitive);            str = str.trimmed();            if (str == ip)                return true; // found        }    }    return false; // not found}void SSHConfigFile::registerIP(QString ip){    if (!find( ip))    {        // this is new ip, add it to config file        addIP( ip );    }}void SSHConfigFile::addIP(QString ip){    QString addendum = QString(# Added by RigakuSync\n                               Host %1\n                               \tStrictHostKeyChecking no\n                               \tUserKnownHostsFile=/dev/null\n).arg(ip);    fileData = \n + addendum;    write();}void SSHConfigFile::write(){    QTextStream out(&file);    out << fileData;    file.flush();    file.close();}Sample use for additional context:void DeviceSync::syncDevice(QString ip){    // register IP to ssh config file if not already there.    // I chose to do it on in function than to first check here if    // this ip exist and if not than add it..fair enough?    SSHConfigFile().registerIP( ip );    // and then the rest of the code to perform sync. Basically i need to     // make sure the ip is there in config file before i perform sync}From the code, I only call the registerIP() function, which is the only public function. It automatically adds the IP if it's not already in the config file.When I pass the IP address, should the class just store it internally and then work with it, or is passing in between member functions better? Also, do the various member functions make sense or could it all be done in just one function?Is writing a class for this a good approach or can this whole thing just be a function in another class? If this class is a good approach, can it be improved?
Reading an SSH config file for an IP
c++;object oriented;qt;ssh
I see a few things that may help you improve your code.Pass by const reference where practicalIn every case that a QString ip is passed, the code could instead use a const QString &ip avoiding pointless multiple copies and also allowing the addition of a const IP address.Consider large filesReading the entire file into memory using QTextStream::readAll() can consume a lot of memory if the file is large. If you still want to do this anyway, see the next suggestion.Don't store the file contents in the objectThe object currently stores both the entire contents of the file in two forms: both as QString and as QStringList which isn't really needed.  First, you could pass the QString back as the return value for read(), and then you could have find(), addIP() and write() take that QString as an additional parameter.  Naturally, this should also be passed by const reference.Reduce object creation where practicalWithin the addIP() call, it isn't really necessary to prepend the \n to the addendum QString since that could easily simply be added to the static string.  Using all of the previous suggestions and this one, here is one way to write this function:void SSHConfigFile::addIP(const QString &ip, const QString &fileData){    QString addendum = QString(\n# Added by RigakuSync\n                               Host %1\n                               \tStrictHostKeyChecking no\n                               \tUserKnownHostsFile=/dev/null\n).arg(ip);    write(fileData + addendum);}Consider an alternative class designThe configuration file for your program is really a collection of attributes that each apply to a single Host, so what might make more sense as you expand this program, would be to create a HostConfig class to hold each individual configuration set, and then your SSHConfig class could implement a collection of those.  List all required #includesI'm sure it is in your code (because it wouldn't otherwise compile) but it would be useful for reviewers to see a full, compilable sample which includes the required #include files.  Sanitize inputAt the moment, if an ip QString with leading whitespace is handed to the registerIP() call, it will always add a duplicate entry because the un-trimmed input ip is being compared to the trimmed partial line.  If I were to abuse the program like this, I could make the configuration file grow without limit.  SSHConfigFile sh1;for (int i=1000000; i; --i)    sh1.registerIP(  8.8.8.8 );Make sure to match file open and closeIn the case that the find routine finds a duplicate entry, it leaves the file open.  If it adds a new entry, it leaves the file closed.  This makes it impossible for the calling program to know whether the file is open or closed at the end of the call.  To fix that, (and using the previous suggestions), you could refactor like this:void SSHConfigFile::registerIP(const QString &ip){    QString fileData = read();    if (!find( ip, fileData))    {        // this is new ip, add it to config file        addIP( ip, fileData );    } else {        file.close();    }}OOP designSimply stated, in Object Oriented Programming, the nouns in a description become objects and the verbs become methods.  In this case the description is reads an SSH config file and checks whether an IP exists. If the IP does not exist, the class adds a block for that IP address. so the relevant nouns are:SSH config fileIP addressblock (for IP address)Verbs are:read filecheck file for IP addressadd block for IP address to fileThese match pretty closely to the methods in your class, and the IP address is passed in from the calling program.  As I mentioned in a previous point, one could consider creating an alternative design in which each block is a Host object, and the file could be implemented as a host collection (perhaps std::map<IPAddress, Host>) but the decision to do so or not depends on how you're planning to use this.  If the need for the IP block is only transient and static (as you've written it) then it's fine as you have it.  If, on the other hand, other parts of the program will be altering settings in the file or looking them up frequently, the Host object approach may be more natural.  Generally speaking, I think about the interface first.  Ask yourself How would I like to interact with this thing?  Then implement that, revealing as few internal details as practical.  Reducing the scope of variables to be as small as possible also helps, so that instead of the old style:int i;for (i=0; i < MAX_STUFF; ++i) { ... }We prefer:for (int i=0; i < MAX_STUFF; ++i) { ... }Similarly, for the IP address, we know that it must be passed in to do the work of the registerIP() method.  If it were part of the object, it would uselessly take up memory as long as the object exists.  However, by passing it by const reference (my first suggestion) you minimize its scope to be as small as possible.  For this design, that's a good choice.  However, if you decide to implement as a collection of Host objects, the IP address would necessarily be member data.