patterncppMinor
Simple framework for Google Code Jam problems
Viewed 0 times
simplegoogleforcodeproblemsframeworkjam
Problem
My main concern is code style. Could you review this?
#pragma once
#include
#include
#include
#include
#include
#include
#include
#include
#include
#include
#include
#include "InfInt.h"
#include "Array2d.h"
template
ContainerT splitString(const std::string & source) {
ContainerT cont;
std::stringstream ss(source);
typedef typename ContainerT::value_type ElementT;
std::copy(std::istream_iterator(ss), std::istream_iterator(), std::back_inserter(cont));
return cont;
}
template
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
T readLine() {
std::string line;
std::getline(input, line);
T list = splitString(line);
return list;
}
std::vector readInts() {
return readLine>();
}
std::vector readStrings() {
return readLine>();
}
std::vector readDoubles() {
return readLine>();
}
void readln() {
readLine();
}
template
void readln(T & t) {
input >> t;
input.ignore();
}
template
void readln(First & first, Rest &... rest) {
input >> first;
readln(rest...);
}
template
void writeResult(T value) {
output << "Case #" << curCase << ": " << value << std::endl;
};
};Solution
#pragma onceThis 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
ContainerT splitString(const std::string & source) {
ContainerT cont;
std::stringstream ss(source);
typedef typename ContainerT::value_type ElementT;
std::copy(std::istream_iterator(ss), std::istream_iterator(), std::back_inserter(cont));
return cont;
}I think I'd use something a bit simpler, such as:
template
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 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.
Code Snippets
#pragma oncetemplate <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 ContainerT>
ContainerT splitString(const std::string & source) {
std::stringstream ss(source);
ContainerT cont{std::istream_iterator(ss), std::istream_iterator()};
return cont;
}ContainerT cont((std::istream_iterator(ss)), std::istream_iterator());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;
}
}
};Context
StackExchange Code Review Q#44494, answer score: 5
Revisions (0)
No revisions yet.