5
\$\begingroup\$

I wrote a C++ program for review and got feedback that I had not used any kind of extensibility of object orientation.

I am wondering if this is a clearly defined concept or one that is more in the eye of the beholder? How would you define using extensibility of object orientation?

My sample program took text and parsed it into words to produce an index of how many times each word was repeated in the text as well as the locations of each word as a sentence number. It then printed out the index. Abbreviations were to be included in the index, but other punctuation omitted (e.g. commas and colons). Upper and lower case versions of words were treated as the same word. I saw no need for virtual functions in solving this nor for class inheritance. For example, this paragraph would result in:

    a       1:1
    abbreviations   1:3
    an      1:1
    and     3:1,3,4
    as      4:1,1,1,4
    be      1:3
    but     1:3
    case        1:4
    class       1:5
    colons      1:3
    commas      1:3
    e.g.        1:3
    each        2:1,1
    example     1:6
    for     3:5,5,6
    functions   1:5
    how     1:1
    i       1:5
    in      4:1,3,5,6
    included    1:3
    index       3:1,2,3
    inheritance 1:5
    into        1:1
    it      2:1,2
    locations   1:1
    lower       1:4
    many        1:1
    my      1:1
    need        1:5
    no      1:5
    nor     1:5
    number      1:1
    of      3:1,1,4
    omitted     1:3
    other       1:3
    out     1:2
    paragraph   1:6
    parsed      1:1
    printed     1:2
    produce     1:1
    program     1:1
    punctuation 1:3
    repeated    1:1
    result      1:6
    same        1:4
    sample      1:1
    saw     1:5
    sentence    1:1
    solving     1:5
    text        2:1,1
    the     5:1,1,2,3,4
    then        1:2
    this        2:5,6
    times       1:1
    to      2:1,3
    took        1:1
    treated     1:4
    upper       1:4
    versions    1:4
    virtual     1:5
    was     1:1
    well        1:1
    were        2:3,4
    word        3:1,1,4
    words       2:1,4
    would       1:6

Please critique just my header file and how it could have been better written as object oriented to solve this problem.

My header file looked like the following:

class Indexer {
private:
  class Entry {
  private:
    typedef vector<unsigned> Occurrences;

  public:
    Entry(const unsigned sentence);
    Entry &operator+=(const unsigned sentence);
    void print(ostream &out) const;

  private:
    unsigned getCount() const;

  private:
    Occurrences _occurrences;

    friend ostream &operator<<(ostream &out, const Entry &rhs);

  };

  class Abbreviations {
  public:
    Abbreviations();
    bool isAbbreviation(const string &word) const;

  private:
    unordered_set<string> _abbreviations;

  };

  class Token {
  private:
    enum punctuationKind {
      none,
      sentenceEnd,
      doubleQuotes,
      otherPunctuation
    };

  public:
    Token(const string &token);
    const string &word() const;
    bool empty() const;
    bool isSentenceEnd() const;

  private:
    void noteSentenceEnd(bool isEnd =true);
    void dropTrailingPunctuation();
    bool isAbbreviation();
    punctuationKind trailingPunctuationKind() const;
    void stripTrailingPunctuation();
    void stripLeadingPunctuation();
    void lowerCase();

  private:
    string _word;
    bool _isSentenceEnd;
    bool _sentenceIsDetermined;

  private:
    static Abbreviations _abbreviations;

  };

  typedef unordered_map<string, Entry> wordMap;

private:
  Indexer();
  Indexer &operator+=(const string &word);
  Indexer &operator+=(Token &token);
  void print(ostream &out) const;

public:
  static void processInput(istream &in =cin, ostream &out =cout);

private:
  wordMap _entries;
  unsigned _currentSentence;

  friend ostream &operator<<(ostream &out, const Indexer &rhs);
  friend ostream &operator<<(ostream &out, const Entry &rhs);

};

Let me know if you think the implementation is necessary to answer this question and I will add it.


Some of the requested implementation:

The Operator +=:

Indexer::Entry::Entry &
Indexer::Entry::operator+=(const unsigned sentence)
{
  _occurrences.push_back(sentence);
  return *this;
}

Indexer::Indexer&
Indexer::operator+=(const string &word)
{
  string strippedWord = word;
  wordMap::iterator itr = _entries.find(word);

  if (itr == _entries.end()) {
    _entries.insert(wordMap::value_type(word, Entry(_currentSentence)));
  } else {
    Entry &curValue = itr->second;

    curValue += _currentSentence;
  }

  return *this;
}

The Token class:

void
Indexer::Token::lowerCase()
{
  const unsigned size = _word.size();

  if (size) {
    static const string uppercaseLetters = "ABCDEFGHIJKLMNOPQRSTUVWXYZ";
    const bool isOnlyFirstLetterUppercase
      = (size == 1 || string::npos == _word.find_first_of(uppercaseLetters, 1));
    char &firstChar = _word[0];

    if (isupper(firstChar) && isOnlyFirstLetterUppercase) {
      firstChar = tolower(firstChar);
    }
  }
}


Indexer::Token::Token(const string &token)
  : _word(token),
    _isSentenceEnd(false),
    _sentenceIsDetermined(false)
{
  stripTrailingPunctuation();
  stripLeadingPunctuation();
  lowerCase();
}


const string &
Indexer::Token::word() const
{
  return _word;
}


bool
Indexer::Token::empty() const
{
  return _word.empty();
}


bool
Indexer::Token::isSentenceEnd() const
{
  return _isSentenceEnd;
}

void // private
Indexer::Token::noteSentenceEnd(bool isEnd /* =true */)
{
  if (!_sentenceIsDetermined) {
    _isSentenceEnd = isEnd;
    _sentenceIsDetermined = true;
  }
}


void // private
Indexer::Token::dropTrailingPunctuation()
{
  _word.erase(_word.size() - 1);
}


bool // private
Indexer::Token::isAbbreviation()
{
  return _abbreviations.isAbbreviation(_word);
}


Indexer::Token::punctuationKind
Indexer::Token::trailingPunctuationKind() const
{
  const unsigned wordLen = _word.size();

  if (0 == wordLen) {
    return none;
  }

  const unsigned lastIndex = wordLen - 1;
  const char lastChar = _word[lastIndex];

  switch (lastChar) {
    case '.':
    case '?':
    case '!': {
      return sentenceEnd;
    }
    case '"': {
      return doubleQuotes;
    }
    case ')':
    case ']':
    case '`':
    case '\'':
    case ',':
    case ';':
    case ':': {
      return otherPunctuation;
    }
  }
  return none;
}


/// Strips all trailing punctuation from a token except for the periods of an
/// abbreviation which are considered part of the word and retained.
void
Indexer::Token::stripTrailingPunctuation()
{
  const punctuationKind punctuation = trailingPunctuationKind();
  const bool havePunctuation = none != punctuation;

  if (havePunctuation && !isAbbreviation()) {
    if (punctuation == sentenceEnd) {
      noteSentenceEnd(true);
    } else if (punctuation == otherPunctuation) {
      noteSentenceEnd(false);
    } else {
      // The punctuation is doubleQuotes so keep looking for sentence end when
      // we recurse.
    }
    dropTrailingPunctuation();
    stripTrailingPunctuation(); // Recurse to find more trailing punctuation.
    return;
  }
}


/// Strips all leading punctuation from a token.
void
Indexer::Token::stripLeadingPunctuation()
{
  static const string leadingPunctuation = "([`\\\"";

  string::size_type firstNonPunctuation
    = _word.find_first_not_of(leadingPunctuation);

  // Erase all leading characters that are not punction marks.
  _word.erase(0, firstNonPunctuation);
}

Additional user of Token:

void // static
Indexer::processInput(istream &in /* =cin */, ostream &out /* =cout */)
{
  Indexer indexer;
  string token;

  while (in >> token) {
    Token currentToken(token);

    indexer += currentToken;
  }

  out << indexer << endl;
}
\$\endgroup\$
  • \$\begingroup\$ I think seeing how the Token members are used and seeing the Indexer::operator+= methods might help a lot. \$\endgroup\$ – Paul Martel Feb 2 '12 at 16:12
  • \$\begingroup\$ @Paul Martel I've added more code per your request. \$\endgroup\$ – WilliamKF Feb 2 '12 at 16:21
  • \$\begingroup\$ Here is a facet to help you ignore punctuation automatically: stackoverflow.com/questions/6154204/… \$\endgroup\$ – Martin York Feb 2 '12 at 20:54
2
\$\begingroup\$

The code shown implements only operator+=(word) but calls only operator+=(token). Is one defined trivially in terms of the other?

Unless you could reasonably expect some requirement for indexer += token; to do something different from indexer += token.word(), OR if you expected to have to replicate all that extra finger typing in MANY calls, go with the latter more explicit code -- it's clearer. On the other hand, it may be more OOPish for Token to have its own operator>> overload so that Indexer is not exposed to how a Token gets read. This change would dumb down the Token constructor to a minimal default constructor (called once) and move the current constructor actions into the operator>> implementation.

As for class hierarchy and virtuals, the one place I could picture it being applied to the current code is as a substitute for punctuationKind. Each of your enum values could be a different class derived from PunctuationKind and its effects on the Token could be encoded in a virtual function called like so:

void
Indexer::Token::stripTrailingPunctuation()
{
    // get the correct singleton.
    const PunctuationKind& punctuation = trailingPunctuationKind();
    if (punctuation.setSentenceState(*this)) { // false for PunctuationNone or abbrev.?
        dropTrailingPunctuation();
        stripTrailingPunctuation(); // Recurse to find more trailing punctuation.
    }
}

Overkill? Possibly. Maybe not if you reasonably expect to have to build additional handling for other punctuation with new and different behavior.

BTW, does your code properly handle abbreviations that have extra trailing punctuation?

That leaves only OOP for the sake of future code. The question to ask is what new requirements could be reasonably expected and how could the existing code be set up so that it would still meet the requirements.

So, factor out a base class where there is a reasonable expectation that some specific members or methods of a class will be useful under some generalized set of requirements while the other specific members or methods of that class will not be useful.

Make a function virtual wherever you reasonably expect that expanded requirements will call for different instances of a class to implement it differently when driven by the same calling code and when the choice of behavior can and should be decided when the instance is constructed.

Other kinds of requirement changes are better anticipated by generic programming techniques (templates), or by other kinds of refactoring (into referenced components).

All of these things have their place in OOP.

If you don't have a sense for how the requirements are likely to change, so that you have a clear idea of why you are applying any of these techniques, then you are better off keeping things simple -- fewer lines written means fewer lines to rewrite.

\$\endgroup\$
  • \$\begingroup\$ I do not hear anything in your review to correlate with my failing review. Bottom line, in your opinion, why would the above preclude me from further consideration for an OOP lead position due to failure to use any extensibility of object oriented software? I wish to address this particular deficiency in my solution. \$\endgroup\$ – WilliamKF Feb 3 '12 at 15:59
1
\$\begingroup\$

First of all, code seams to be overly bloated. You do not need such a huge amount of classes for your simple task. Most of your methods may be written as (static) functions or even inlined into places of their use.

Next, there may be possible an object orientation.

   class Index;
   class Entry; // some virtual methods here
   class Abbreviation: public Entry;
   class Word: public Entry;

   Entry* get_entry( ...... );

Your get_entry function shall analyze an input stream and return either new Abbreviation(....) or new Entry(....); use Index constructor instead of processInput, and print result using Index::print(ostream&) method or operator<<(Index&) -- that is, separate index creation and printing.

A choice of taste -- do not use inner classes here. A better way is to place all your classes into Indexer namespace.

These 4 classes ( + std::map + std::string + std::vector ) is enough.

\$\endgroup\$
  • \$\begingroup\$ The suggested inheritance serves no apparent purpose, and ties together 3 classes with very different functions to give an impression that they are interchangeable in some hypothetical caller that might call '// some virtual methods'. No. \$\endgroup\$ – Paul Martel Feb 9 '12 at 0:24
  • \$\begingroup\$ @PaulMartel [Sorry for incorrect message that I've deleted]. OOP means extensibility for the future. Future client of Index class may iterate over Index elements; client may need to treat Abbreviation and Word differently. Say, client may need to print index entries for words only and skip abbreviations completely. So, client may quckly call a virtual method to distinguish between words and abbreviations; may be visitor is better solution. Sometimes such a simple solution as map<string> instead of Index class is permittable; note my wording "may be possible an object orientation". \$\endgroup\$ – user1123502 Feb 9 '12 at 14:07
1
\$\begingroup\$

I want to see data structures and logic separated from parsing and string representation.

As such you would be able to use a different method of laying out the data and only have to modify your token parsing or printing logic, not the classes themselves that hold the data structures.

The class should exist whilst allowing more than one string representation / layout.

Work on the grounds of data structure and algorithms and then write external parsers and printers that can parse/populate your structure or read/print them.

This is where I see your design lacking extensibility.

\$\endgroup\$
  • \$\begingroup\$ Good point. I agree in principle, but how would this be worked here in practice? Token already seems to be doing a pretty good job of encapsulating the parsing and spoon-feeding entries to Indexer, except maybe for the mysterious updating of _currentSentence -- a little glitch that I missed in my review. For output, we can tell from the lack of something like iterator accessors on Indexer (even just private ones) that the operator<< is directly accessing the representation. Do you think that would provide enough separation or is a more radical change in order? \$\endgroup\$ – Paul Martel Feb 9 '12 at 1:36

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.