Is this a good implementation

[aymanbah]

Hi, I need to know if the implementation of operator<< is a good implementation ?
if no, can I get explanation y its not ?

#ifndef _MOVIE_H_
#define _MOVIE_H_

#include <string>
#include <ostream>

using namespace std;

class Movie {
public:
	Movie(string name, int score);

	int getScore() const;
	string getName() const;
	
	/* movie1 is smaller than  movie2,
	*  if its score is smaller. 
	*  If the scores are the same, their names should be lexicographic compared*/
	bool operator<(const Movie& ) const;
	
	/* movie1 equals movie2 if their scores and names are the same */
	bool operator==(const Movie&) const;

	/* Should output the movie name and movie score */
	friend ostream& operator<<(ostream&, const Movie&);
private:
	int _score;
	string _name;
};

#endif 

#include <iostream>
#include <string>
#include <ostream>
#include "Movie.h"	

using namespace std;

	ostream& operator<<(ostream& out, const Movie& movie) {
		cout << movie._name << "(" << movie._score << ")." << endl;
		return out;
	}

Thanks a lot.

[kooth]

I think it's great! I use this often.

[aymanbah]

Thnx man.

[Duthomhas]

Er, get it out of the std namespace...

[aymanbah]

I`m using another std functions, so it`s easier to use namespace.
I know it takes a lot of memmory, but for this HW is okay to use.
is there another problem using std namespace ?

[Duthomhas]

Yes, it doesn't belong to you and you don't need to modify it. If your professor is like me he'll deduct points for doing something like that.

Using stuff in a namespace != modifying that namespace.

[jsmith]

Well I notice that the ostream parameter is "out", but the function uses "cout" instead.

[ne555]

Er, get it out of the std namespace...

¿do you mean global? ¿or are you referring to line 7 in the header?

@OP: ¿what are the getters for?

[shacktar]

In my opinion, it's bad style to use the using directive in a header. This is because the using directive cannot be undone, so any module that #include s that header, must also (possibly unknowingly) use that namespace, leading to potential conflicts.

aymanbah wrote:

I know it takes a lot of memmory

AFAIK using something like using namespace std; does not consume more memory (or change the object code at all) relative to that statement not being present. It does not "pull in" the namespace, but rather, it causes std:: to be affixed to any symbol that cannot be found in the current namespace. using namespace std; does not cause the current module to place any symbols into the std namespace.

Duoas wrote:

Using stuff in a namespace != modifying that namespace.

The OP is not modifying the namespace, though. This would be modifying it:

#ifndef _MOVIE_H_
#define _MOVIE_H_

#include <string>
#include <ostream>

namespace std {

     class Movie {
          ...
     };
}

#endif 

#include <iostream>
#include <string>
#include <ostream>
#include "Movie.h"	

namespace std {

     ostream& operator<<(ostream& out, const Movie& movie) {
          cout << movie._name << "(" << movie._score << ")." << endl;
          return out;
     }

}

Though I agree that using namespace std; is lazy and could lead to conflicts, it gives me the most concern when it's used in a header.

[Alrededor]

It may be best not to include endl in your Movie class output operator. Instead, if you have a collection of movies (perhaps an array or vector) and want to output the collection, one movie per line, put the endl character inside the loop that outputs the collection as follows:

std::vector< Movie > movieHits;
// code to populate movieHits
.
.
.

for ( int i = 0; i < movieHits.size(); ++i )
{
    std::cout << movieHits[ i ]  << std::endl;
}

Why?

Suppose you later create a class call TheaterItem.

class TheaterItem;
{
public:
    .
    .
private:
    Movie movie_;
    DateTime startTime_;
    .
    .
    .
};

You would probably want the output operator for the TheaterItem class to output both the movie and the starting time on the same line perhaps separated by a semicolon.

std::cout << movie_ << ';' << startTime_;

This way if you had a collection of TheaterItems, say a schedule of future theater movies, you could easily output each one on its own line within a loop:

std::vector< TheaterItem > schedule;
// code to populate schedule
.
.
.

for ( int i = 0; i < schedule.size(); ++i )
{
    std::cout << schedule[ i ]  << std::endl;
}

which would give output like:

Rise of the Planet of the Apes(23).;08/15/2011-16:00
Cowboys & Aliens(15).; 08/15/2011-19:00

By hard coding the endl character in your Movie class you make it impossible to do the above. For similar reasons hard coding the period after the score is problematic.

Also, note again jsmith's comment about using cout instead of the out parameter.

[aymanbah]

Also, note again jsmith's comment about using cout instead of the out parameter.

What is this meaning ?
the implementaion should be :

	ostream& operator<<(ostream& cout, const Movie& movie) {
		cout << movie._name << "(" << movie._score << ")." << endl;
		return cout;
	}

[Alrededor]

I only used cout in my examples because that is what you used. jsmith's comment was in reference to line 9 of your code:

cout << movie._name << "(" << movie._score << ")." << endl;

This should have been

out << movie._name << "(" << movie._score << ")." << endl;

[Duthomhas]

shacktar wrote:

The OP is not modifying the namespace, though.

So the OP is declaring a function in the std namespace but somehow he is not modifying it? [edit] I could've sworn that I saw a namespace std { ... in his code.

The only reason you can't tell the difference is because of Koenig lookup -- which searches the available namespaces -- including the one available because of using namespace std;.

@aymanbah
Your function should be:

ostream& operator<<(ostream& out, const Movie& movie) {
	out << movie._name << "(" << movie._score << ").";
	return out;
}

Now you can use it the normal way:

Movie mymovie( "The Illusionist", 5 );
cout << mymovie << endl;

Also, as shacktar said, don't ever use using namespace anything; in header files (there is only one obscure exception -- and that is if the purpose of the header is to include specific namespaces -- and you still have to do it just right).

Hope this helps.

[1vRz3TCk]

Duoas wrote:

[edit] I could've sworn that I saw a namespace std { ... in his code.

In the second piece of code? That is where I saw it...but the post does not appear to have been edited.

[Duthomhas]

Oh, so I'm not hallucinating? I thought I must have made a sleep-deprived error exactly because the post does not appear edited. Can an OP edit his original posts without showing that it was?

[1vRz3TCk]

It was odd for me because I could have sworn I saw the definition of the function in the std namespace. I remember think it was out of place...reading on I saw your post about it..and then I saw a post saying it was not in the std namespace. I scrolled back up and saw it as it is.

I decided it was time for a coffee (or two).

I think that it is probably a combination of the way the code is formatted and skim reading it. :0(

[moorecm]

What I recommend is making a namespace for each class that has a public interface that includes functions outside of the class. A class's public interface can be more than its public methods. This logically groups the functionality, supports Koenig lookup, and does not pollute the global namespace.

In this case, I would also use the public get methods for the class rather than declaring the function as a friend.

[Duthomhas]

http://www.cplusplus.com/forum/general/13162/
The only thing I would amend to that post is to use both #pragma once and include guards:

// This file is the interface to your library.

// Use the #define guards instead in addition to #pragma once for maximim portability
#pragma once
#ifndef MYLIB_HPP
#define MYLIB_HPP

...

Disch also made an article covering headers and includes, but he left out stuff about namespaces.
http://cplusplus.com/forum/articles/10627/

Hope this helps.

[kooth]

I completely missed the OP's not using out instead of cout. My bad on that, but it's an easy typo to make.

[Duthomhas]

Ditto.