13
\$\begingroup\$

Please take a look at my program and let me know how I can improve it.

/*
"   To Print A Line On The Display Screen"
    Date:5th January 2011
    Programmer:Fahad
*/
#include <iostream>
using namespace std;
class Print
{
    public:
        void print_();
};
int main()
{
    Print Obj;
    Obj.print_();
    system( "pause" );
    return 0;
}
void Print::print_()
{
    cout << "I am in print function and the program runs fine." << endl;
}
\$\endgroup\$
  • 3
    \$\begingroup\$ I think you misunderstood the idea of using underscore from my previous post. Usually you want to append the underscore to private data members of your class. As others have already pointed out, appending '_' to methods and functions is unconventional and makes it rather arkward for client code to use. \$\endgroup\$ – greatwolf Feb 4 '11 at 18:33
18
\$\begingroup\$

In addition to the other comments, I would also use a different naming convention for types and objects.

For example, this looks unconventional.

Print Obj;
Obj.print_();

I prefer:

Print obj;
obj.print();

It's just a convention but being able to easily spot names that denote types helps if you start to use more complex expressions. For example:

Print().print();

Personally, I would also avoid system("pause"). You need to #include either <stdlib.h> or <cstdlib> to use it. Although the system call itself is standard C++ (from the standard C library), what you pass to it is system dependent.

In general I don't believe you should make your programs stop artificially. If they are designed to run in a terminal then the terminal user will be able to see the output even after the program exits.

\$\endgroup\$
  • 2
    \$\begingroup\$ And if they are designed to run in a terminal, you should run them from a terminal or from a wrapper script which itself holds the terminal open after your program exits. \$\endgroup\$ – Fred Nurk Feb 8 '11 at 23:59
10
\$\begingroup\$

In addition to what Billy said, I find Obj.print_() to look strange in C++ code. I would have just made the method name print().

\$\endgroup\$
9
\$\begingroup\$

Not sure what you mean by coding style. If you're talking about spacing and such, here are changes I would make. Note that these are entirely subjective and people are probably going to disagree with me.

Don't indent the public: specifier in the class -- leave it flush with the curly braces that mark the class definition. The reason for this is the implicit private region in the class.

Example code:

class Example
{
    int a; //Shouldn't this line
    public:
        void MyFunc(); //Indent to the same place this one does?
};


class Ahhhhh
{
    int a;  //Ahhh.. we match now :)
public:
    void MyFunc();
};

For that matter this class has no private members so I would just change class to struct and remove the access specifier entirely.

I would remove using namespace std; and explicitly qualify those members which are in std. Would really stink to get a nasty error message from the compiler because you happened to define a function called copy (which might conflict with std::copy).

system("pause"); should be std::cin.get();

If you're talking about "design", it seems overengineered to me. No reason to involve objects in a program like this at all. Just sticking the print statement in main would suffice.

\$\endgroup\$
  • \$\begingroup\$ I'm assuming the "overengineering" is because it was a homework assignment asking them to make a class and provide a method on it. \$\endgroup\$ – Mark Loeser Feb 4 '11 at 16:35
  • 2
    \$\begingroup\$ Personally, I prefer that private be explicit too. Easier on my eyes. \$\endgroup\$ – Michael K Feb 4 '11 at 17:15
  • \$\begingroup\$ @Mark: Probably. However I mention this because of the huge number of people who avoid free functions and think EVERYTHING_MUST_BE_AN_OBJECT which I see all the time coming from Java people. If that's required for the homework assignment then of course it's fine lol. \$\endgroup\$ – Billy ONeal Feb 5 '11 at 18:16
7
\$\begingroup\$

The one thing that is not C++ like for me is this:

Obj.print_();

This tightly couples the print method to a particular output method.
It would be better to allow the user of your object to define what the output method is:

std::cout << Obj << "\n";

Which means you need to define an output operator for you object:

std::ostream& operator<<(std::ostream& str, Print const& data)
{
    // STUFF
    return str;
}

Though not technically wrong. I am not a fan of underscore at the ends of identifiers:

print_()

Looks wierd to me. But this is a style thing. See you local coding conventions for rules. If you had put it on the front I would have been a lot more complainey about it.

System is hard to use cross platform. Especially when you do system("pause").

system( "pause" );

I prefer the platform neutral:

std::cout << "Hit Enter to continue\n";
std::cin.clear();
char plop;
std::cin >> plop;  // cin is buffered. So nothing is sent until you hit enter.
\$\endgroup\$
6
\$\begingroup\$

Under the circumstances, a using directive seems highly suspect. While there are times/places that it's useful, this doesn't seem (to me) to be one of them.

The name of a typical class should also be a noun, not a verb. A verb signals that what you have is a single action, which is not a good candidate for a normal class. If it's going to be a class at all, it should probably be a functor. I'd also add a parameter (with a default value) so it would be easy to use a stream other than std::cout when/if necessary:

struct Print { 
    std::ostream &operator()(std::ostream &os = std::cout) { 
        return os << "whatever\n";
    }
};

Using system("pause"); is also quite non-portable. If you want to wait for the user to press a key before ending the program, it's generally better to build that into your own code:

void pause() { 
    std::cout << "Press \"enter\" when ready.\n";
    getchar();
}

int main() { 
    Print()();
    pause();
    return 0;
}

Frankly, even using the functor strikes me as silly in this case though -- you're taking something simple (print out a string) and making it much more complex without getting anything in return. Given how little the program does, the Print class accomplishes nothing useful or positive at all.

\$\endgroup\$
5
\$\begingroup\$

I prefer seeing class names that are nouns and method names that are verbs. Print might read better as Printer, ObjectPrinter or WhateverPrinter.

I've seen the _ suffix (or a m_ prefix) to denote members quite a bit but I've never found it useful to attach this sort of decoration to a name.

\$\endgroup\$
5
\$\begingroup\$

I'm really not a fan of putting the main() function in the middle. I prefer it to be the first thing in the file, or the last thing in the file. I would personally have defined your Print class in a header file, put the actual code for it a separate source file, and then put your main function in the main source file... This would make your Print class a lot easier to reuse in another application or turn into a library.

For example

Print.h

#ifndef __PRINT_H__
#define __PRINT_H__

class Print
{
    public:
        void print_();
};

#endif //__PRINT_H__

Print.cpp

#include <iostream>
#include "Print.h"

void Print::print_()
{
    std::cout << "I am in print function and the program runs fine." << std::endl;
}

main.cpp

/*
"   To Print A Line On The Display Screen"
    Date:5th January 2011
    Programmer:Fahad
*/
#include <stdlib.h>
#include "Print.h"

int main()
{
    Print Obj;
    Obj.print_();
    system( "pause" );
    return 0;
}
\$\endgroup\$
  • \$\begingroup\$ Definitely prefer to place your classes in separate source files. It will allow you to reuse code in other projects. It will also help you to think about separating concerns, and can reduce dependencies when you start making large numbers of classes. If everything is in the same file, everything must be recompiled when that one file changes. \$\endgroup\$ – YoungJohn Dec 12 '13 at 22:55
0
\$\begingroup\$

The concept of printing is better defined as a method of an object, for instance of the World class. For example, we can define the following:

#include <iostream>
int main(int argc, char* argv[]){
    class World{
    public:
        World(void){
            std::cout << "Hello " << __FUNCTION__ << "\n";
        }
    } Hello;
}

The constructor will print the message as soon as the World object is instantiated as Hello. It is more common to define the class outside of the main, but not forbidden...

\$\endgroup\$
  • 1
    \$\begingroup\$ If you're going to make a broad statement like The concept of printing is better defined as a method of an object, please explain why it is better. Is it more efficient? Does it better meet some Software Design Principal? Your answer is a little lacking and needs to be improved. \$\endgroup\$ – pacmaninbw Aug 29 '16 at 0:45
  • \$\begingroup\$ Thank you for giving me the opportunity to refine my reply. My answer is concise because I prefer to stay to the point and avoid distractions, but in this case, we are dealing with a methodology, Object Oriented Programming, that aims at creating a model and defining how it works. To remain within the spirit of the methodology, we gain in clarity by representing verbs, like "print", as being what the object does, and using nouns to describe behavior. This is why I named the class of the object "World". \$\endgroup\$ – Pierre C Aug 29 '16 at 1:14

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy