Skip to main content
Code Review

Return to Answer

replaced http://stackoverflow.com/ with https://stackoverflow.com/
Source Link

Putting using namespace std at the top of every program is a bad habit a bad habit that you'd do well to avoid. In my own production code, I usually simply type std:: where needed. It's a little bit more typing but it has two very nice benefits: it absolutely avoids any possibility of name collisions and it makes it absolutely clear which particular namespace is being used for various items.

All versions of both standards since then (C99 and C++98) have maintained the same idea. We rely on automatically generated member functions in C++, and few people write explicit return; statements at the end of a void function. Reasons against omitting seem to boil down to "it looks weird" "it looks weird". If, like me, you're curious about the rationale for the change to the C standard read this question read this question. Also note that in the early 1990s this was considered "sloppy practice" because it was undefined behavior (although widely supported) at the time.

Putting using namespace std at the top of every program is a bad habit that you'd do well to avoid. In my own production code, I usually simply type std:: where needed. It's a little bit more typing but it has two very nice benefits: it absolutely avoids any possibility of name collisions and it makes it absolutely clear which particular namespace is being used for various items.

All versions of both standards since then (C99 and C++98) have maintained the same idea. We rely on automatically generated member functions in C++, and few people write explicit return; statements at the end of a void function. Reasons against omitting seem to boil down to "it looks weird". If, like me, you're curious about the rationale for the change to the C standard read this question. Also note that in the early 1990s this was considered "sloppy practice" because it was undefined behavior (although widely supported) at the time.

Putting using namespace std at the top of every program is a bad habit that you'd do well to avoid. In my own production code, I usually simply type std:: where needed. It's a little bit more typing but it has two very nice benefits: it absolutely avoids any possibility of name collisions and it makes it absolutely clear which particular namespace is being used for various items.

All versions of both standards since then (C99 and C++98) have maintained the same idea. We rely on automatically generated member functions in C++, and few people write explicit return; statements at the end of a void function. Reasons against omitting seem to boil down to "it looks weird". If, like me, you're curious about the rationale for the change to the C standard read this question. Also note that in the early 1990s this was considered "sloppy practice" because it was undefined behavior (although widely supported) at the time.

Source Link
Edward
  • 67.2k
  • 4
  • 120
  • 284

I see some things that may help you improve your code.

Don't abuse using namespace std

Putting using namespace std at the top of every program is a bad habit that you'd do well to avoid. In my own production code, I usually simply type std:: where needed. It's a little bit more typing but it has two very nice benefits: it absolutely avoids any possibility of name collisions and it makes it absolutely clear which particular namespace is being used for various items.

Make sure you have all required #includes

The code uses std::isalpha but doesn't #include <cctype> or <locale>. It's not clear which one you want here. The functions in <cctype> match the functions you're using, but you should be aware that they are only defined for the C locale. Read this for details.

Omit unused variables

Because word is never used, it can and should be omitted from the program.

Use better naming

I would say that words is a good variable name, but line (singular) is not. Also, we have countletters (plural) but countnum (singular). Some consistency in naming would improve this program.

Initialize and open files in one step

Instead of separately invoking lab3.open() I'd suggest instead writing it this way:

std::ifstream lab3{"lab3.txt"};

Or if you wanted to make the program much more flexible and allow the user to specify a file name, this could be:

std::ifstream lab3{argv[1]};

Decompose the program into smaller parts

Right now, all of the code is in main which isn't necessarily wrong, it means that it's not only hard to reuse but also hard to troubleshoot. Better is to separate the code into small chunks. It makes it both easier to understand and easier to fix or improve. In this case, I'd suggest creating an object that looks like this:

class WordCounter 
{
public:
 WordCounter();
 void count(std::istream &in);
 friend std::ostream& operator<<(std::ostream &out, const WordCounter &w);
private:
 int letters;
 int nums;
 int puncts;
 int spaces;
 int words;
 int lines;
};

When you define those functions, then, the main routine can look like this:

int main()
{
 std::ifstream lab3{"lab3.txt"};
 if (!lab3)
 {
 std::cout << "Could not open file\n";
 return 1;
 }
 WordCounter counter;
 counter.count(lab3);
 std::cout << counter;
}

Use consistent formatting

Using consistent formatting helps readers of your code understand it without distraction. This code is mostly well formatted, but the indenting within the while loop is a bit inconsistent.

Use precise terminology

The code claims to be counting sentences, but is actually counting lines. Either number will work if no lines contain more than one sentence, but I did not see any guarantee that this will always be the case.

Don't use std::endl if you don't really need it

The difference betweeen std::endl and '\n' is that '\n' just emits a newline character, while std::endl actually flushes the stream. This can be time-consuming in a program with a lot of I/O and is rarely actually needed. It's best to only use std::endl when you have some good reason to flush the stream and it's not very often needed for simple programs such as this one. Avoiding the habit of using std::endl when '\n' will do will pay dividends in the future as you write more complex programs with more I/O and where performance needs to be maximized.

Omit return 0

When a C or C++ program reaches the end of main the compiler will automatically generate code to return 0, so there is no need to put return 0; explicitly at the end of main.

Note: when I make this suggestion, it's almost invariably followed by one of two kinds of comments: "I didn't know that." or "That's bad advice!" My rationale is that it's safe and useful to rely on compiler behavior explicitly supported by the standard. For C, since C99; see ISO/IEC 9899:1999 section 5.1.2.2.3:

[...] a return from the initial call to the main function is equivalent to calling the exit function with the value returned by the main function as its argument; reaching the } that terminates the main function returns a value of 0.

For C++, since the first standard in 1998; see ISO/IEC 14882:1998 section 3.6.1:

If control reaches the end of main without encountering a return statement, the effect is that of executing return 0;

All versions of both standards since then (C99 and C++98) have maintained the same idea. We rely on automatically generated member functions in C++, and few people write explicit return; statements at the end of a void function. Reasons against omitting seem to boil down to "it looks weird". If, like me, you're curious about the rationale for the change to the C standard read this question. Also note that in the early 1990s this was considered "sloppy practice" because it was undefined behavior (although widely supported) at the time.

So I advocate omitting it; others disagree (often vehemently!) In any case, if you encounter code that omits it, you'll know that it's explicitly supported by the standard and you'll know what it means.

lang-cpp

AltStyle によって変換されたページ (->オリジナル) /