tags:

views:

517

answers:

12

(Leaving aside the question of should you have them at all.)

I have always preferred to just use function overloading to give you the same name for both getter and setters.

int rate() { return _rate; }      
void rate(int value) { _rate = value; }

// instead of     
int getRate() { return _rate; }      
void setRate(int value) { _rate = value; }

// mainly because it allows me to write the much cleaner     
total( period() * rate() );    
// instead of      
setTotal( getPeriod() * getRate() );

Naturally I am correct, but i wondered if the library writers had any good reason ?

+14  A: 

I would prefer the get/set versions because it is more clear as to what is going on. If I saw rate() and rate(10), how do I know that rate(10) isn't simply using 10 in the calculation to return the rate? I don't, so now I have to start searching to figure out what is going on. A single function name should do one thing, not two opposing things.

Also, as others have pointed out, some prefer to omit the 'get' and leave the 'set', i.e.,

int Rate( );
void SetRate( int value );

That convention is pretty clear as well, I wouldn't have any problem reading that.

Ed Swangren
Good point. Assume the person using your code isn't just a homicidal maniac who knows where you live, but is also a bit confused!
Martin Beckett
+1  A: 

While Ed's comment is true, I do prefer actual properties over the setter/getter antipattern. When 1/3rd of the methods in your domain graph are dummy methods that have been generated by eclipse, there's something wrong.

Without first class properties, however, I believe the antipattern makes the most sense.

Furthermore, it makes code completion easier.

obj.set (control shift space) for setters
obj.get (control shift space) for getters

Stefan Kendall
Why is it an "antipattern" when first class properties are simply syntactic sugar for the same thing? the concept is the same, it is simply the implementation that differs.
Ed Swangren
It's an antipattern in the sense that it could be a one liner.private property int value; //now works with IoC containers; Yay."I repeat 30 lines of code at the top of all my functions. How is this an antipattern? The code works!"
Stefan Kendall
I think he means having getters/setters at all is an anti pattern -which isn't necessarily true - see my comment to jmucchiello
Martin Beckett
+5  A: 

I have always preferred to omit the 'get' on my getters, as you do, with rate() instead of getRate(). But overloading for the setter does not seem like a very good idea to me, since the name rate doesn't convey that the object is being mutated. Consider:

total(period() * rate()); // awesome, very clear

rate(20); // Looks like it computes a rate, using '20'...but for what?  And why ignore the return value?
David Seiler
this is the Qt and java style. http://qt.gitorious.org/qt/pages/ApiDesignPrinciples ---> goto "the art of naming"
Ronny
+5  A: 

How about int rate(); and void setRate(int value);? This has the virtue of not having two functions of the same name doing different things, and still allows period() * rate().

David Thornley
Would probably lead to confusion with people looking for getXXX
Martin Beckett
this is the Qt and java style. http://qt.gitorious.org/qt/pages/ApiDesignPrinciples ---> goto "the art of naming"
Ronny
+3  A: 

A few years ago, I would have agreed completely. More recently, a doubt began to make its way, because that makes taking the address of a getter or setter ambiguous. With tools like tr1::bind, this is really annoying.

For example:

struct A
{
   void Foo(int);
   int Foo()const;
};

std::vector<A> v = ....;
std::vector<int> foos;
// Extract Foo
std::transform(
   v.begin(), v.end(), 
   std::back_inserter(foos), 
   //Ambiguous
   // std::tr1::bind(&A::Foo)
   //must write this instead. Yuck!
   std::tr1::bind(static_cast<int(Foo::*)()>(&A::Foo));
);

Leaving aside the question of should you have them at all ;-)

Éric Malenfant
bind() and even worse bind2nd() are the things that make me thing C++ is irretrievably broken. No human being can reasonably be expected to parse, let alone write, that line of code correctly!
Martin Beckett
UncleBens
+4  A: 

I'll go ahead and mention this should be a community wiki question.

When I started learning C++ I looked for style guides, and Google's was good for some points:

  • Methods in uppercase (it's just prettier).
  • getters plainly and lowecase (rate).
  • setters explicitly and lowercase (setRate).
Tordek
ok - it was just an idle coffee break sort of thought.
Martin Beckett
+1  A: 

Being concise is important, but not at the cost of being incomplete or misleading. For that reason, I prefer GetFoo() and SetFoo() to Foo() and Foo(int foo).

dicroce
A: 

Personally, I think getters and setters found in pairs are a code smell carried over from "visual" languages and their "properties". In a "good" class, the data members are writeonly or readonly but not read/write.

I think the most common cause of getters and setters is not carrying the object model deep enough. In your example, why is total being passed the period and the rate? Aren't they members of the class? So you should only be setting the period and the rate and you should only be getting a total.

There are probably exceptions but I just hate looking at a class and finding "getX/setX, getY/setY, etc. etc." It just seems there wasn't enough thought put into how the class SHOULD be used and rather the author made the class EASY to get at the data so he wouldn't have to consider how the class should be used.

Naturally I am correct.

jmucchiello
Think of a 3d point type. It will have lots of internal functions to do various maths stuff, but is ultimately going to need all 6 get/set x/y/z functions.
Martin Beckett
@mgb: personally I don't believe a 3d point type should hide the members in the first place (except perhaps because you might want to use an array as the internal representation).
UncleBens
A: 

There are several levels to "Getting" and "Setting"

  • I use Get and Set for "fast" operations.
  • If something will take a longer time to execute, then it will often be a Calc, as these names imply that some work has to be done to retrieve the result.
  • For longer operations, you start to get into prefixes like Load/Save, Query/Store, Read/Write, Search/Find etc.

So Get/Set can be ascribed a useful meaning, and be part of a larger, consistent naming strategy.

Jason Williams
A: 

I enforce the convention in which a method should always be a verb and a class should always be a noun (except for functors, for obvious reasons). In which case, a get/set prefix must be used for consistency. That said, I also agree entirely with Ed Swangren. This sums to me as using those prefixes a no-brainer.

Fabio Ceconello
+1  A: 

Another issue no one else has mentioned is the case of function overloading. Take this (contrived and incomplete) example:

class Employee {
    virtual int salary() { return salary_; }
    virtual void salary(int newSalary) { salary_ = newSalary; }
};

class Contractor : public Employee {
    virtual void salary(int newSalary) {
        validateSalaryCap(newSalary);
        Employee::salary(newSalary);
    }
    using Employee::salary; // Most developers will forget this
};

Without that using clause, users of Contractor cannot query the salary because of the overload. I recently added -Woverloaded-virtual to the warning set of a project I work on, and lo and behold, this showed up all over the place.

Tom
You don't happen to know the equivalent to -Woverloaded-virtual for MSVC?
Greg Domjan
@Greg sorry, I don't. I actually find MSVC's warnings (as of VC8) to be mostly counter-productive (performance warning for conversion between bool and int? struct previously forward-declared as class?) Since I build on two platforms, I usually rely on gcc for all my warnings needs.
Tom
A: 

I prefer to avoid the get and set labels, the information is not needed for the compiler to do it's job for most of these simple properties.

you can have issues:

class Stuff {
  void widget( int something ); // 'special' setter
  const Widget& widget( int somethingelse ) const; // getter
}
Stuff a; 
a.widget(1); // compiler won't know which widget you mean, not enough info
Greg Domjan