5
\$\begingroup\$

I'm a C#/Java full-time developer who is trying to pick up C++ purely for educational reasons. I've tried to solve a pretty simple ACM ICPC problem and would love to hear all kinds of criticisms on how to write acceptable modern C++ code. (Seriously, tell me that my code is a joke, as long as you can tell me how to make it better.)

Specifically, I'd like to hear comments on the following aspects:

  1. Does the code follow typical C++ coding convention?
  2. Is it cross platform? Especially regarding how I am handling strings.
  3. Is it C++11 compliant? Am I making a reasonable use of it?
  4. Is the code readable?

You're also welcome to comment on my algorithm on solving the problem, although that's less of a concern for me at the moment because the primary purpose for me is to learn how to properly use C++.

Full source (VS2012)


The challenge is to determine whether each line of input contains a "jolly jumper" sequence:

A sequence of n > 0 integers is called a jolly jumper if the absolute values of the difference between successive elements take on all the values 1 through n-1. For instance,

1 4 2 3

is a jolly jumper, because the absolutes differences are 3, 2, and 1 respectively.

Below is a test input you can try running against the program. The program works as intended as far as I can tell.

4 1 4 2 3
5 1 4 2 -1 6
10 1 2 3 4 5 6 7 8 9 10
10 1 2 4 7 11 16 22 29 37 46
10 -1 -2 -4 -7 -11 -16 -22 -29 -37 -46
10 -1 -1 -4 -7 -11 -16 -22 -29 -37 -46
1 1
2 1 2
2 2 1
4 0 4 2 3
4 1 3 2 4
1 2
6 1 4 3 7 5 10

main.cpp

#if defined(WIN32) || defined(_WIN32) || defined(__WIN32) && !defined(__CYGWIN__) 
    #define WIN32_DEF 1
#else
    #define WIN32_DEF 0
#endif

#if WIN32_DEF
#include "win32helper.h"
#endif

#include "jolly_jumper.h"

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

using std::cin;
using std::cout;
using std::endl;
using std::string;
using std::ifstream;

int main(int argc, char *argv[], char *envp[]) {
    #if _DEBUG && WIN32_DEF
        //Visual studio does some wacky things with setting current directory when ran under debug mode..
        //This could be changed in the project settings, but might as well as add code here to
        //manually control current working directory...
        cout << "[Running under debug mode]" << endl;
        setCurrentDirectoryForVSDebug();
    #endif

    string fileName = "";
    if (argc > 1) {
        fileName = string(argv[1]);
    }
    else {
        cout << "Input file name: ";
        getline(cin, fileName); 
    }

    while (fileName.empty()) {
        cout << "Filename cannot be empty!" << endl;
        cout << "Input file name: ";
        getline(cin, fileName); 
    }

    cout << "Reading from: " << fileName << endl;

    ifstream fileStream(fileName);
    if (!fileStream || !fileStream.is_open()) {
        cout << endl << fileName << " could not be opened. Make sure the file exists and the read permission has been set correctly.";
        return 0;
    }

    JollyJumper jollyJumper;
    string currLine = "";
    while (getline(fileStream,currLine)) {
        cout << currLine << endl;   
        if (jollyJumper.IsJolly(currLine)) {
            cout << "Jolly" << endl;
        }
        else {
            cout << "Not Jolly" << endl;
        }
    }

    fileStream.close();
    cin.get();
    return 0;
}

string_helper.h

#ifndef __STRING_HELPER_H
#define __STRING_HELPER_H

#include <vector>
#include <string>
#include <istream>
#include <sstream>
#include <algorithm>
#include <cctype>
#include <iostream>

//Static Class Declaration
class StringHelper {
private:
    StringHelper() {};
public:
    static std::vector<std::string> &Split(const std::string &s, char delim, std::vector<std::string> &elems);
    static std::vector<std::string> Split(const std::string &s, char delim);
    static bool IsNumber(const std::string& s);

};


#endif

jolly_jumper.h

#ifndef __JOLLY_JUMPER_H_ 
#define __JOLLY_JUMPER_H_

#include <iostream>
#include <string>
#include <vector>
#include <set>

class JollyJumper
{
private:

public:
    JollyJumper();
    bool IsJolly(std::string input);
};

#endif

win32helper.h

#ifndef __WIN32_HELPER_H_
#define __WIN32_HELPER_H_

#include <windows.h>
#include <tchar.h>
#include <Shlwapi.h>
#include <iostream>

void setCurrentDirectoryForVSDebug();

#endif

jolly_jumper.cpp

#include "jolly_jumper.h"
#include "string_helper.h"

using std::string;
using std::vector;
using std::cout;
using std::endl;
using std::set;

#define MAX_INPUT_VALUE 2000

JollyJumper::JollyJumper()
{
}

bool JollyJumper::IsJolly(string input)
{
    if (input.empty())
        return false;

    vector<string> elements = StringHelper::Split(input, ' ');
    if (elements.size() == 1)
        return true;

    //First value determines the count of input
    if (!StringHelper::IsNumber(elements.front()))
        return false;

    int inputLength = stoi(elements.front());

    if (inputLength > MAX_INPUT_VALUE)
        return false;

    if (elements.size() - 1 != inputLength)
        return false;

    //Skip first element since it only tells us the size of the elements
    auto it = elements.begin();
    it++;

    //Validate string inputs and make sure they are integers.
    //Once validated, push the difference of curr iterator and next iterator to jolly_diff vector
    set<int> jolly_diff;
    for (; it != elements.end(); it++) {        
        if (!StringHelper::IsNumber(*it)) {
            return false;
        }

        int value = abs(stoi(*it));

        auto nx = std::next(it);
        if (nx == elements.end()) {
            break;
        }

        if (!StringHelper::IsNumber(*nx)) {
            return false;
        }

        int nextValue = abs(stoi(*nx));

        int difference = abs(value - nextValue);

        //Difference cannot logically be greater than the input length
        if (difference >= inputLength)
            return false;

        //Duplicate difference means it is not a jolly jumper
        if(jolly_diff.find(difference) != jolly_diff.end()) {
            return false;
        }
        jolly_diff.insert(difference);
    }

    return true;
}

string_helper.cpp

#include "string_helper.h"

using std::vector;
using std::string;
using std::getline;
using std::stringstream;

vector<string>& StringHelper::Split(const string &s, char delim, vector<string> &elems) {
    stringstream ss(s);
    string item;
    while (getline(ss, item, delim)) {
        elems.push_back(item);
    }
    return elems;
}


vector<string> StringHelper::Split(const string &s, char delim) {
    vector<string> elems;
    Split(s, delim, elems);
    return elems;
}

bool StringHelper::IsNumber(const std::string& s)
{
    std::string::const_iterator it = s.begin();
    if(s.size() > 1 && (s[0] == '-' || s[0] == '+')) it++;
    while (it != s.end() && std::isdigit(*it)) ++it;
    return !s.empty() && it == s.end();
}

win32helper.cpp

#include "win32helper.h"

using std::wcout;
using std::endl;

void setCurrentDirectoryForVSDebug() {
        TCHAR currentFilePath[MAX_PATH];
        //Get the full path including the file name from the executable
        GetModuleFileName(NULL, currentFilePath, MAX_PATH);
        //Remove filename and get directory only
        PathRemoveFileSpec(currentFilePath);

        wcout << "Setting current directory to: " << currentFilePath << endl << endl;
        SetCurrentDirectory(currentFilePath);
}
\$\endgroup\$
4
\$\begingroup\$

I'll just focus on your jolly_jumper.h:

#ifndef __JOLLY_JUMPER_H_ 
#define __JOLLY_JUMPER_H_

#include <iostream>
#include <string>
#include <vector>
#include <set>

class JollyJumper
{
private:

public:
    JollyJumper();
    bool IsJolly(std::string input);
};

#endif

Issues I see include:

  • Pay attention to const correctness. The parameter should be const, and the method should be const.
  • The method is going to be doing too much: it needs to parse the input string as a vector, then run its algorithm. The IsJolly() method should take a vector (as a const std::vector&).
  • Your class is "degenerate". There's no state. Your constructor does nothing. You might as well write it as a standalone function, or perhaps a class that contains a static function.
  • The private keyword is just noise.

In summary, I suggest:

struct JollyJumper
{
    static bool IsJolly(const std::vector &input);
};
\$\endgroup\$
  • 1
    \$\begingroup\$ or even: namespace JollyJumper \$\endgroup\$ – YoungJohn Sep 11 '14 at 19:45
3
\$\begingroup\$

In addition to @YoungJohn observations:

  • class JollyJumper

    A class with a single method and no state is just a noise to displease Occam. IsJolly should be made free.

  • IsNumber()

    Seems redundant. stoi(), among other things, returns an index of a first unconverted character; to test that the whole string is converted compare it to string length.

  • Testing for duplicates.

    A call to find() is redundant: set::insert() returns a pair<iterator, bool>, where the second is false when you inserting a duplicate.

\$\endgroup\$
3
\$\begingroup\$

At first glance the bits that strike me as odd here involve your StringHelper class. Since it is a class with nothing but static members it would probably be better suited as a C++ namespace.

A namespace gives you essentially the same functionality as a class with nothing but static members, but better stresses your intent. Also, namespaces unlike classes can be re-opened and added to in separate modules giving them some versatility.

It also seems strange to me that you have a Split method in your StringHelper class with this declaration:

static std::vector<std::string> &Split(const std::string &s, char delim, std::vector<std::string> &elems);

It both takes a parameter elems which appears to be an output parameter, and it returns a value which in the implementation happens to also be elems. I would say that you should either avoid the output parameter and return a std::vector<std::string> by value or return void and use elems the parameter as the sole output of the method. Again this would clarify your intent. Having both a return value and an output parameter suggests that there are two important and different outputs from this function.

Furthermore, it seems that that version of Split is merely an implementation detail of the other Split declared as:

static std::vector<std::string> Split(const std::string &s, char delim)

Since the first Split function appears to be an implementation detail and not used or intended as part of the StringHelper public interface, it shouldn't be a public member function. If you leave StringHelper as a class, you might make that Split a private member. If you make StringHelper a namespace the non-public version of Split would be better as a free function in an unnamed namespace in the implementation file (.cpp) for StringHelper with no declaration at all in the header file.

\$\endgroup\$

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.