6
\$\begingroup\$

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_OBJECT
public:
    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 false
bool 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?

\$\endgroup\$
  • \$\begingroup\$ Such info, especially after answers, isn't needed and just clutters up the question. If you'd like to address answerers, comment below their posts. \$\endgroup\$ – Jamal Nov 7 '15 at 3:52
6
+50
\$\begingroup\$

I see a few things that may help you improve your code.

Pass by const reference where practical

In 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 files

Reading 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 object

The 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 practical

Within 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 design

The 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 #includes

I'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 input

At 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 close

In 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 design

Simply 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 file
  • IP address
  • block (for IP address)

Verbs are:

  • read file
  • check file for IP address
  • add block for IP address to file

These 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.

\$\endgroup\$
  • \$\begingroup\$ Thanks really appreciate your take. To make sure the file is closed, I have used exceptions on class level, would that be good approach to make sure file is closed? In that case the find function will in try block and if there is error, it will close the file. The exception will not be visible outside of this class. \$\endgroup\$ – zadane Oct 30 '15 at 2:32
  • \$\begingroup\$ could you also speak to the points I raised on the other answer (by janos) regarding the class design and store ip in the class? Thanks. \$\endgroup\$ – zadane Oct 30 '15 at 2:37
  • \$\begingroup\$ I've added an "OOP design" commentary to my answer. \$\endgroup\$ – Edward Oct 30 '15 at 12:32
  • \$\begingroup\$ Thanks its helpful. I have really been focusing on how to design a class lately and what actually it represents and how it should operate. I have written a blog entry (which really is just notes fro myself) but here is the link below, that might give you clue where I am coming from. awamisoft.wordpress.com/2015/09/09/class-design-setting-it-up \$\endgroup\$ – zadane Oct 30 '15 at 13:37
4
\$\begingroup\$

Your main questions

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?

You mean, you should pass data around as function parameters, or store inside fields. In your example, storing in fields doesn't really make sense. Currently you have this:

  • The read function reads the contents into a field
  • The find function checks if the IP address is contained in the field
  • The write function writes the field back to a file

The flow of the data would be more direct and easier to follow if you passed it in function parameters instead of in fields.

Storing data in fields makes sense if you need to store state. In your example, you don't really store state.

Also, do the various member functions make sense or could it all be done in just one function?

It's good to decompose programs to small units, such as functions. You could even go further.

Is writing a class for this a good approach or can this whole thing just be a function in another class?

The class seems to be a good candidate for an ADT (abstract data type). It represents a configuration file, and provides a method to register an entry safely. The implementation correctly hides the details of how it really works.

It's good to decompose programs to functions that have a single responsibility. As the tasks you need take several steps, it makes sense to perform those steps in separate functions, each with a single responsibility. However, the functions that perform the steps are implementation details that should not be exposed to users. Therefore, it's good that you wrapped those operations in a class, and made the helper methods private.

Design of the functions

Calling registerIP will trigger if (find -> read) -> write. For one thing, it re-reads the entire config file every time. Which could be acceptable. Unfortunately, only the write method closes the file, and this method is not called if the given IP is already in the file. Failing to close open file handles is a common big mistake. In this program it's even worse that multiple unclosed file handles can accumulate. You need to reorganized the methods in a way that the file gets closed even when not writing a new IP address to it.

A related issue is opening a file for rewriting. You might not need to write to the file at all, if the IP is already registered. So it would be better to open for reading only. In fact you don't need to rewrite the entire content. You only ever append lines, so you could re-open for appending when needed.

Error handling

If the config file could not be open and read returns early, what will happen to the rest of the program? If find doesn't crash, it will return false, and then write will try to write to a file handle that's not open. This doesn't look good.

\$\endgroup\$
  • \$\begingroup\$ The first question you picked is important to me. I mean when we pass a parameter to member function of a class, which will later be used by other member functions as well (internally), Is it better that the first function saves it to the class so other methods can use it later or it should keeping passing them internally in the class? \$\endgroup\$ – zadane Oct 29 '15 at 15:31
  • \$\begingroup\$ I rewrote the first point, I hope it answers your question \$\endgroup\$ – janos Oct 29 '15 at 18:15
  • \$\begingroup\$ I added comments in the OP in response. \$\endgroup\$ – zadane Oct 29 '15 at 20:39
  • 1
    \$\begingroup\$ OOP is not only about using data in fields. And in this example there's no good reason to use data in fields. If you want to practice using data in fields, pick a more typical OOP problem. As I explained, the dependency between your functions is unclear without looking at the implementation, which is not good, and avoidable \$\endgroup\$ – janos Oct 29 '15 at 20:57
  • \$\begingroup\$ I added more comments to OP what I mean to be OOPy. \$\endgroup\$ – zadane Oct 30 '15 at 15:39

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.