current community

your communities

more stack exchange communities

Stack Exchange
tour help
Take the 2-minute tour ×
Code Review Stack Exchange is a question and answer site for peer programmer code reviews. It's 100% free, no registration required.
up vote 3 down vote favorite

My main concern is code style. Could you review this?

#pragma once

#include <time.h>
#include <iostream>
#include <fstream>
#include <sstream>
#include <string>
#include <vector>
#include <iterator>
#include <algorithm>
#include <functional>
#include <math.h>
#include <exception>
#include "InfInt.h"
#include "Array2d.h"

template <typename ContainerT>
ContainerT splitString(const std::string & source) {
    ContainerT cont;
    std::stringstream ss(source);
    typedef typename ContainerT::value_type ElementT;
    std::copy(std::istream_iterator<ElementT>(ss), std::istream_iterator<ElementT>(), std::back_inserter(cont));
    return cont;
}

template <typename T>
std::string joinString(const T & v, const std::string & delim = " ") {
    std::ostringstream s;
    bool empty = true;
    for (const auto & i : v) {
        if (empty)
            empty = false;
        else
            s << delim;
        s << i;
    }
    return s.str();
}

class CodeJam {
public:
    std::stringstream input;
    std::stringstream output;
    int curCase;

    int solveJam(const std::string & data) {

        int res = 0;
        std::cout << "Solving jam:" << std::endl << std::endl;
        try {
            input.clear();
            output.clear();
            auto inputFile = "Data\\" + data + ".in";
            std::ifstream finput(inputFile);
            if (!finput.is_open())
                throw std::runtime_error("Can not open input file " + inputFile);
            auto outputFile = "Data\\" + data + ".out";
            std::ofstream foutput(outputFile);
            if (!foutput.is_open())
                throw std::runtime_error("Can not open output file " + outputFile);
            input << finput.rdbuf();
            input.seekg(0);

            auto elapsedTime = clock();
            res = solveAll();
            elapsedTime = clock() - elapsedTime;

            foutput << output.rdbuf();
            finput.close();
            foutput.close();

            std::cout << std::endl;
            std::cout << "Success! Finished " << res << " tests in " << elapsedTime << " ms" << std::endl;
        } catch (const std::exception & e) {
            std::cout << "Error: " << e.what() << std::endl;
        }
        std::cout << std::endl;
        system("pause");

        return res;
    }   
protected:

    virtual int solveAll() {
        int testCases;
        readln(testCases);

        for (curCase = 1; curCase <= testCases; ++curCase) {
            solve(curCase);
            std::cout << "Test " << curCase << " of " << testCases << std::endl;
        };  
        return testCases;
    }

    virtual void solve(int task) = 0;

public:
    std::string readLine() {
        std::string line;
        std::getline(input, line);
        return line;
    }

    template<typename T>
    T readLine() {
        std::string line;
        std::getline(input, line);
        T list = splitString<T>(line);
        return list;
    }

    std::vector<int> readInts() {
        return readLine<std::vector<int>>();
    }

    std::vector<std::string> readStrings() {
        return readLine<std::vector<std::string>>();
    }

    std::vector<double> readDoubles() {
        return readLine<std::vector<double>>();
    }

    void readln() {
        readLine();
    }

    template <typename T> 
    void readln(T & t) {
        input >> t;
        input.ignore();
    }

    template <typename First, typename... Rest> 
    void readln(First & first, Rest &... rest) {
        input >> first; 
        readln(rest...);
    }

    template <typename T>
    void writeResult(T value) {     
        output << "Case #" << curCase << ": " << value << std::endl;
    };
};
share|improve this question
    
You probably want a virtual destructor there (stackoverflow.com/questions/461203/…). –  Mat Mar 16 at 9:09
    
Keep all your public and private methods together. Right now you have two blocks of public methods. This is confusing. Also generally speaking, public member variables are frowned upon. –  Emily L. Mar 16 at 11:00

1 Answer 1

up vote 5 down vote 
#pragma once

This really only makes sense in a header, but the rest of your code doesn't really look like a header (but maybe it's intended to be one anyway--not really sure). If it is a header, I'd recommend adding normal #ifndef/#define/#endif style header guards as well, since some compilers don't support this #pragma.

template <typename ContainerT>
ContainerT splitString(const std::string & source) {
    ContainerT cont;
    std::stringstream ss(source);
    typedef typename ContainerT::value_type ElementT;
    std::copy(std::istream_iterator<ElementT>(ss), std::istream_iterator<ElementT>(), std::back_inserter(cont));
    return cont;
}

I think I'd use something a bit simpler, such as:

template <typename ContainerT>
ContainerT splitString(const std::string & source) {
    std::stringstream ss(source);
    ContainerT cont{std::istream_iterator(ss), std::istream_iterator()};
    return cont;
}

If you're stuck with C++03 instead of 11, you'll need to use:

    ContainerT cont((std::istream_iterator(ss)), std::istream_iterator());

...instead (note the double parens around the first param, to avoid the Most Vexing Parse).

As @Mat already alluded to in a comment, the fact that this contains a pure virtual function indicates that it's intended to be used as a base class for derivation. If so, you almost certainly want to make the destructor virtual as well--this is needed if the user might ever create a pointer to the base class, and destroy an instance via that pointer to the base.

To be honest, this scenario strikes me as somewhat unlikely though. Such a virtual function is generally useful when you have something like a heterogeneous container that might contain pointers to objects of any of a number of derived classes, so the correct function to call must be determined individually for each item.

In this case, it appears more likely (at least to me) that the intent is to have only one derived class per program, so what we really have is "static polymorphism" -- i.e., in a given program, there will be only one derived class, so one program will really only have one implementation of solve.

If that's correct, it would probably be better to model that a bit more directly, such as by passing the solve as a template parameter instead:

template <class solve>
class CodeJam {
// ...
    void solveAll() { 
        // ...
        for (curCase = 1; curCase <= testCases; ++curCase) {
            // Note that the syntax is only minimally affected:
            solve()(curCase);
            std::cout << "Test " << curCase << " of " << testCases << std::endl;
        }
    }
};

This seems to improve flexibility at least somewhat, because the user is no longer required to write a class that's derived from CodeJam.

At the same time, I feel obliged to point out what seems to me an even greater problem (and one that's not so easily fixed either, I'm afraid). This comes down to the simple fact that your code simply doesn't seem to provide all that much functionality. I can hardly imagine how anybody could hope to save even 10 whole minutes by using your code rather than starting from nothing at all--and in some cases, I can see where this code might easily be no more than a break-even proposition.

In short, you're placing constraints on how code is written, but providing rather minimal functionality in return. I question whether it provides enough return on investment to justify itself.

share|improve this answer

Your Answer

 
discard

By posting your answer, you agree to the privacy policy and terms of service.

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