7
\$\begingroup\$

Having written C# for half a year now, I decided to take on a fairly complex first challenge for C++: the Is it simple or it is hard? challenge from Code Golf Stack Exchange. (This is only practice, not an entry to the contest; my code is ~2,500 bytes so honestly submitting it would be a joke.)

The rules of the contest are:

Take a sequence of strings as input or as command line arguments (I did the latter), read in some words from a file, and check whether all of the words entered (case insensitive / removed punctuation / dashes delimit words) are contained within that file.

Here is my VC++ 'solution':

stdafx.h

#include "targetver.h"

#include <stdio.h>
#include <tchar.h>

#include <algorithm>
#include <cctype>
#include <exception>
#include <fstream>
#include <iostream>
#include <sstream>
#include <stdexcept>
#include <string>
#include <vector>

PlainSpeak.cpp

#include "stdafx.h"

using std::cin;
using std::cout;
using std::endl;
using std::exception;
using std::find;
using std::getline;
using std::ifstream;
using std::mbstowcs;
using std::runtime_error;
using std::string;
using std::stringstream;
using std::transform;
using std::vector;
using std::wistringstream;
using std::wstring;

vector<wstring> *vectptr = 0; // use ptr because NULL is valid

bool simple(const wstring &word);
wstring& format(const wstring &word);
vector<wstring>& read_dict(const string &path);
vector<wstring>& split(const wstring &text, wchar_t delimiter);

int _tmain(int argc, _TCHAR* argv[])
{
    bool complex = false;

    for (int i = 1; i < argc; i++)
    {
        const wstring &word = argv[i];

        vector<wstring> vect = split(word, L'-');

        for (int i = 0; i < vect.size(); i++)
            if (!simple(vect[i]))
                complex = true;
    }

    cout << endl << "Your phrase was " << \
        (complex ? "complex." : "simple.") << \
        endl << endl << "Press any key to continue... ";

    cin.get();
}

bool simple(const wstring &word)
{
    if (vectptr == 0)
        vectptr = &read_dict("dict.txt");

    wstring formatted = format(word);

    return find(vectptr->begin(), vectptr->end(), word) != vectptr->end(); // check if vector contains word
}

wstring& format(const wstring &word)
{
    wstring wstr = word;
    transform(wstr.begin(), wstr.end(), wstr.begin(), tolower);

    for (int i = 0; i < wstr.length(); i++)
    {
        wchar_t wch = wstr[i];

        if (wch < 97 || wch > 122) // if not letter...
            wstr[i] = L' '; // replace with whitespace
    }

    return wstr.replace(wstr.begin(), wstr.end(), L" ", L""); // remove all whitespace
}

vector<wstring>& read_dict(const string &path)
{
    ifstream reader(path);

    if (!reader.good())
        throw runtime_error("File " + path + " not found.");

    stringstream buf;
    buf << reader.rdbuf();
    string str = buf.str(); // no need to call reader.close(), this is handled by RAII

    // convert str to a wstring
    wstring wstr(str.size(), L' ');
    wstr.resize(mbstowcs(&wstr[0], str.c_str(), str.size()));

    return split(wstr, L' ');
}

vector<wstring>& split(const wstring &text, wchar_t delimiter)
{
    wistringstream reader(text);
    wstring curr;
    vector<wstring> result;

    while (getline(reader, curr, delimiter))
        result.push_back(curr); // equivalent to List<T>.Add in C#

    return result;
}

Some things I'd like to know about my code:

  • Can it be shortened? (Obviously 2.5k bytes just to check if a file contains a sequence of words is non-optimal.)

  • Did I use references too much / too little?

    • Does it matter if a local variable is a reference or not?
    • Does it matter if a method returns a reference or not?
  • Is there anything I could have done better? For example, could I have replaced mbstowcs with another function?

  • Should I have / have not used forward declarations for the methods other than _tmain? Do they make my code clearer or more verbose?

  • Any other thoughts / comments you have.

\$\endgroup\$
3
  • 1
    \$\begingroup\$ This is not an answer, anyway for comparison my Ruby solution is about 400 bytes. \$\endgroup\$
    – Caridorc
    Commented Jul 19, 2015 at 18:09
  • \$\begingroup\$ @Caridorc Nice; you should try submitting to the question on Code Golf. I'm just looking for suggestions on how to shorten my code; C++ doesn't have as many higher-level operators (to my knowledge) like select which are present in C# and Ruby, and even lowercasing a string is painful. \$\endgroup\$
    – James Ko
    Commented Jul 19, 2015 at 18:21
  • \$\begingroup\$ I could save 100 or 150 bytes more, still I am not compiting with the others. \$\endgroup\$
    – Caridorc
    Commented Jul 19, 2015 at 18:24

2 Answers 2

6
\$\begingroup\$

The implementation is very inefficient:

  • The words are in a vector, so checking if a word exists takes linear time. The right data structure for this is a set
  • As far as the main method is concerned, once a word doesn't exist in the dictionary, the complex flag is set to true and cannot change, so you could stop processing at that point immediately

The implementation has very strange elements:

  • The methods are poorly named: "simple" doesn't convey the meaning of searching
  • The "simple" method shouldn't be in charge of reading in the dictionary content. That should happen in the caller, which would also naturally avoid the strange null check on the dictionary vector
  • It's confusing to use loop variable i inside another loop that also uses variable i

The forward declarations are fine: it makes it clear what functions are defined within the file, and hint at the program structure right at the top, so that's not a bad thing.

\$\endgroup\$
1
  • \$\begingroup\$ Thanks for the advice! I'm surprised that the naming conflict with i even compiled; and yes, I did not see those inefficiencies in my implementation. Personally, though, I thought that the forward declarations and the usings were verbose; maybe I should move them into a header. \$\endgroup\$
    – James Ko
    Commented Jul 20, 2015 at 0:03
2
\$\begingroup\$
  • Can it be shortened? (Obviously 2.5k bytes just to check if a file contains a sequence of words is non-optimal.)

    Yes, your code can be shortened, but you have to be careful not to sacrifice readability in the process.

  • Does it matter if a local variable is a reference or not?

    You should not be returning references to local variables or variables declared on the stack due to the way stack frames work. In any case, it does not matter if a local variable is a reference or not

  • Does it matter if a method returns a reference or not?

    This depends on what the method caller wants in return. If the expected return value is a reference, then this method better do that.

  • Is there anything I could have done better?

    I will answer this at the end...

  • Should I have / have not used forward declarations for the methods other than _tmain? Do they make my code clearer or more verbose?

    Forward declaration usage is good and since you have just a few methods, it doesn't make the code any harder to read. If you had more methods, then a header file would have worked better.

  • Any other thoughts / comments you have.

    All this comes down to is knowing your standard library and all the tools it has to offer, and having an idea of the right data structures to use


  • Is there anything I could have done better? (Cont...)

    As I mentioned before, this comes down to knowing the in's and out of the standard library. Your read_dict method can be reduced to this:


std::vector<std::wstring> read_dict(const std::string& path) {
    std::wifstream in(path);
    std::vector<std::wstring> vect{std::istream_iterator<std::wstring>{in},
        std::istream_iterator<std::wstring>{}};
    return vect;
}

As a result, this also eliminates the need for the split method. Check out the c++ algorithms library for more useful functions that you can use to eliminate most of your code

\$\endgroup\$
1
  • \$\begingroup\$ Thank you for your advice on not returning local variables as references; I had no idea about this until I got a warning copy-pasting into cpp.sh. Up until now, I thought there was no syntactic difference between a reference and the actual thing because it's implicitly dereferenced every time you try to use it, but I guess I was wrong. Thanks as well for your advice on considering a header file, and I will definitely be looking to learn more of the standard library after the code sample you've shown me. \$\endgroup\$
    – James Ko
    Commented Jul 20, 2015 at 0:08

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge you have read our privacy policy.

Not the answer you're looking for? Browse other questions tagged or ask your own question.