6
\$\begingroup\$

Other code review noted problems with commenting and non-C++ code. This post aims to follow up on those issues.

Please check:

  1. Comments are appropriate
  2. Code is not C

My test file contains:

one two three four five six seven eight nine

//This app is an exercise/study: Etude fr1_x
// prints x number of words from file, without newline, as determined by
//      short wordCount
// in:
//      void printWord(std::vector<std::string> &word)
// the output is connected to a timer.
#include <iostream>
#include <string>
#include <fstream>
#include <vector>
#include <cstdlib>
#include <unistd.h>

//number of times to repeat output when calling println();
//instead of writing:
//
//  println();
//  println();
//
//i can write:
//
//  println((count)2);
//
//
//also, because println(int i) is already defined
typedef unsigned short count;

//TODO - yank all print[ln] to header
void print(std::string str){
    std::cout << str << std::flush;
    return;
}

void print(char c){
    std::cout << c << std::flush;
    return;
}

void print(int i){
    std::cout << i << std::flush;
    return;
}

void println(std::string str){
    std::cout << str << std::endl;
    return;
}

void println(){
    std::cout << std::endl;
    return;
}

void println(count c){
    const short maxVal = 10;/*short-term control - might assign neg val to count
                              i did, just to test it.*/
    if(c>maxVal){
        println((count)2);//accidental recursion
        println("Error:");
        println("called 'println(count c) with invalid arg");
        println("required: param c<=10");
        exit(0);
    }
    for(int i=0;i<c;i++){
        std::cout << std::endl;
    }
    return;
}

void println(char c){
    std::cout << c << std::endl;
    return;
}

void println(int i){
    std::cout << i << std::endl;
    return;
}

void println(short s){
    std::cout << s << std::endl;
    return;
}

std::string prompt(std::string str){
    std::string prompt = (str + ": ");
    std::string input;
    print(prompt);
    std::cin >> input;
    return input;
}

std::string getFileName(){
    return prompt("\nEnter a file name");
}

//TODO - break it up - too nested - too conditional
void readFile(std::vector<std::string> &word){
    const std::string quit("quit");
    std::string fileName;
    std::string str;
    short kontinue = 1;
    do{
        fileName = getFileName();
        kontinue = fileName.compare(quit);
        if(kontinue != 0){
            std::ifstream file(fileName.c_str(),std::ios::in);
            if(!file.is_open()){
                continue;
            }
            //source:Thinking in C++, 2nd ed. Volue 1, Ch. 2: Introducing vector
            while(file >> str){
                word.push_back(str);
            }
            file.close();
            break;
        };
    }while(kontinue != 0);
    return;
}

void test(std::vector<std::string> &word){
    int size = word.size();//could be a problem, maybe use long
    println();
    for(int i=0;i<size;i++){
        print(word[i] + " ");
    }
    println((count)2);
    for(int i=0;i<size;i++){
        println(word[i]);
    }
    return;
}

void slp(){
    usleep(1000000);//val will become user input eventually.
    return;
}

void carriageReturn(std::string str){
    std::string blank;
    int strSize = str.length();
    for(int i=0;i<strSize;i++){
        blank += " ";
    }
    //inconsistent - not print()
    std::cout << "\r" << blank << std::flush << "\r";
    return;
}

//TODO - this is suspect - may be too complicated - refactor/simplify
void printWord(std::vector<std::string> &word){
    //how many values to print at one time from vector
    short wordCount = 4;//this will become user input eventually.
    short numRead = 0;
    int size = word.size();//could be a problem, maybe use long.
    //even better, inspect file to determine needs

    int rmndr = size % wordCount;
    int stop = (size-rmndr);
    int idx = 0;
    std::string str;
    while(idx < stop){
        str += (word[idx++] + " ");
        ++numRead;
        if(numRead == wordCount){
            print(str);
            slp();
            carriageReturn(str);
            numRead = 0;
            str.clear();
        }
    }
    if(rmndr){
        for(int i=stop;i<size;i++){
            str += (word[i] + " ");
        }
        print(str);
        slp();
        carriageReturn(str);
    }
    return;
}

int main(){
    //TODO - doWhile
    std::vector<std::string> word;
    readFile(word);
    //test(word);
    println();
    printWord(word);
    return 0;
}
\$\endgroup\$
11
\$\begingroup\$

Comments:

  • The top comment block is good. When writing a program, describing it at the top will tell the reader what it is expected to do output.

  • Too much whitespace within some blocks. Some of your comment text are too spread-out, adding more lines and reducing readability. Avoid this by making your comments as concise as possible.

  • Talking to yourself "aloud". Comments written in the first-person could be rewritten more professionally:

    // short-term control - assign negative value to count for testing
    
  • "TODOs": They are okay, but should be kept simple:

    // TODO - break up nested and conditional parts
    
  • "val will become user input eventually" and book references: We don't need to know these things. They only help you, not us.

Code is not C:

Compared to your previous program revision, this does look more C++-like. @Lstor's main point was to use std::string, which you're now doing in place of char arrays.

I'd recommend getting more familiar with C++'s STL. std::string is part of this library, and there's much more to explore. Use it whenever possible, and your code will become more C++-like, more concise, and more robust. Here are two starting points:


Miscellaneous:

  • Your #includes could be sorted alphabetically for organization.

  • Be aware that <unistd.h> is platform-specific. This is not recommended if your program needs to attain portability.

  • By default, void functions return on their own. It's only necessary when the function may need to end early for whatever reason.

  • The "print" functions are unnecessary and just clutter the program. Just use std::cout.

  • In println()'s if-block: It's best to avoid terminating with exit(). Replace it with a return and try to fall back to main() for termination.

  • printWord()'s reference parameter should be const. This applies to any non-native data type (such as an object) that will not be modified within the function.

  • std::getline() is preferred for a user-inputted std::string:

    std::string input;
    std::getline(std::cin, input);
    
  • size() returns an std::size_type, not an int. Correct return types should always be used, and you especially shouldn't mix signed/unsigned types.

    std::vector<std::string>::size_type size = word.size();
    

    You also don't need to create a variable for this. Just use the word.size() in the for-loop.

  • readFile() is hard to follow and could be simplified:

    std::vector<std::string> readFile(std::istream& inFile)
    {
        std::vector<std::string> word;
        std::string fileStr;
    
        while (inFile >> fileStr)
        {
            word.push_back(fileStr);
        }
    
        return word;
    }
    

    It's best to check the file in main() so that it can terminate if the file isn't found. In this function, an input stream (for the file created in main() is received and an std::vector is returned. This is okay to do because of Return Value Optimization.

| improve this answer | |
\$\endgroup\$
  • 1
    \$\begingroup\$ +1, valid points. What do you mean with your last item? string is default-constructed to the empty string. Also, I don't agree with the count item. Count is a noun and is more concrete (and therefore better) than UShort. \$\endgroup\$ – Lstor Jul 24 '13 at 18:20
  • \$\begingroup\$ @Lstor: It is? I thought I once encountered problems on my own with that, but apparently not. I'll take out the other bulletpoint as well. \$\endgroup\$ – Jamal Jul 24 '13 at 18:23
  • 2
    \$\begingroup\$ Regarding TODO: I think it's perfectly valid to keep notes of TODOs in the code. Listing them at the top would make it like a very primitive bug tracker -- keeping them at the code in question makes it metadata. I'd keep them short and sweet, though: // TODO: Read as input. \$\endgroup\$ – Lstor Jul 24 '13 at 18:25
  • 2
    \$\begingroup\$ I disagree about book references as a general comment, but think you're correct about this specific comment. In particular, when you're using a complicated algorithm, a book reference is useful. When you're just using standard library elements as designed, book references are noise. \$\endgroup\$ – ruds Jul 25 '13 at 4:19
  • \$\begingroup\$ @Jamal, "Talking to yourself "aloud". Comments" - I think you nailed it. I work/study alone and that is affecting things - I have to adjust my view. Thank you for your time and work. \$\endgroup\$ – yas Jul 26 '13 at 16:34
6
\$\begingroup\$

I can't help thinking that this seems somewhat...over-engineered. I think I'd consider something on this order as a starting point:

#include <string>
#include <algorithm>
#include <sstream>
#include <iostream>

int main() {
    std::istringstream in("one two three four five six seven eight nine");

    std::cout << std::distance(std::istream_iterator<std::string>(in),
                               std::istream_iterator<std::string>());
}

Here I've embedded your test input into the program, but the same idea applies equally well to an external file.

| improve this answer | |
\$\endgroup\$
  • \$\begingroup\$ "over-engineering" caught my attention; I am glad to have reference to the idea - thank you. From : stackoverflow.com/questions/1001120/… "...Implement things when you actually need them, never when you just foresee that you need them." \$\endgroup\$ – yas Aug 5 '13 at 18:10

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.