Thread: using the C++ sort stl

Originally Posted by phantomotap

No. You don't.

Your original code already exhibits a separation of header (declarations) and source (implementations), and as well separates the functions from the class as non-member functions.

*shrug*

Well, I guess your original code may also fail to match this requirement, but it is much more likely that you are simply imaging a non-existent requirement holding to the notion because you got errors related to access rights. (The inaccessibility of `private' members.)



I'm unfamiliar with the framework, but the implementation for your constructor from your example is already not included in the header; you have only provided a declaration.

Assuming that such is an actual requirement, your only legitimate alternative is `inline' implementations, but before you go that path, you'll probably lose points for going that path.

Soma

Yeah you might be right about that. Sometimes I tend to over think myself if I'm unsure of how to go about coding something.

Below is what the header file looks like.

Code:

class Employee
{
public:
   /**
      Constructs an employee with empty name and no salary.
   */

   Employee();

   /**
      Constructs an employee with a given name and salary.
 
   */

   Employee(string employee_name, double initial_salary);
   
   
   
   set_Employee(string name,double salary);
   
   /**
      Sets the salary of this employee.
      @param new_salary the new salary value
   */

   void set_salary(double new_salary);

   /**
      Gets the salary of this employee.
      @return the current salary
   */
   double get_salary() const;


   /**
      Gets the name of this employee.
      @return the employee name
   */
   string get_name() const;
   
/** 
   The sorting function which compares two employees by name.
 
   @return true if the first employee's salary is less
*/   

  static bool sortByName (Employee a , Employee b);

/** 
   The sorting function which compares two employees by salary.

   @return true if the first employee's salary is less
*/   
  static bool sortBySalary (Employee a , Employee b); 
   
private:
   string name;
   double salary;

We can add in additional functions, variables,etc, to the header file, I just can't change the variable names and parameters (although I might have to double check with the creator of this system just to make sure though)

Originally Posted by FloatingButter

Below is what the header file looks like.

In this case, it looks like manasij7479's suggestion (refer to post #2) was already presented to you by your professor through the header file given.

I presume that you left out the header inclusion guards, headers included and using directive/declaration, and made a typo or two as whiteflags observed. Note the bad habits that your professor appears to have (or perhaps is just catering for you not having been taught them yet, but ugh):

• As mentioned earlier, you should not have a using directive or using declaration at file scope in a header file, so the various appearances of string should be std::string.
• std::string and Employee objects should be passed by (const) reference unless you really want to copy them.
• The sortByName and sortBySalary static member functions are really misnomers because they don't sort: they compare.

Oh, 'setEmployee()' actually isn't suppose to be in there, I added that one in myself.

^ Yes I did leave out the header guards.

This is the exact header file as it was given to me:

Code:

#ifndef CCC_EMPL_H
#define CCC_EMPL_H

#include <string>
using namespace std;


class Employee
{
public:
   /**
      Constructs an employee with empty name and no salary.
   */
   Employee();
   /**
      Constructs an employee with a given name and salary.
      @param employee_name - the employee name
      @param initial_salary - the initial salary
   */
   Employee(string employee_name, double initial_salary);
   
   
   
   
   
   /**
      Sets the salary of this employee.
      @param new_salary the new salary value
   */
   void set_salary(double new_salary);
   /**
      Gets the salary of this employee.
      @return the current salary
   */
   double get_salary() const;
   /**
      Gets the name of this employee.
      @return the employee name
   */
  string get_name() const;
   
/** 
   The sorting function which compares two employees by name.
   @param a The first employee
   @param b The second employee
   @return true if the first employee's salary is less
*/   
 bool sortByName (Employee a , Employee b);
/** 
   The sorting function which compares two employees by salary.
   @param a The first employee
   @param b The second employee
   @return true if the first employee's salary is less
*/   
   bool sortBySalary (Employee a ,  Employee b); 
   
private:
   string name;
   double salary;
  
};



#endif

The version I have now though has the 'std::string' added as well as the 'using namespace std;' removed. I've also added in the 'const Employee& a and b '

Code:

   /**
      Gets the name of this employee.
      @return the employee name
   */
  std::string get_name() const;
   
/** 
   The sorting function which compares two employees by name.
   @param a The first employee
   @param b The second employee
   @return true if the first employee's salary is less
*/   
 bool sortByName (const Employee& a , const Employee& b);
/** 
   The sorting function which compares two employees by salary.
   @param a The first employee
   @param b The second employee
   @return true if the first employee's salary is less
*/   
   bool sortBySalary (const Employee& a , const Employee& b); 
   
private:
   std::string name;
   double salary;

Part of the problem is if you leave the comparison functions in the class declaration then they will need to be invoked on objects.

Code:

Employee e1, e2, e3;
bool isBefore = e1.sortByName(e2, e3);

So you have a situation like the above where the object that invoked the method (e1) isn't or shouldn't be involved in the comparison.

With static member functions you can at least do:

Code:

bool isBefore = Employee::sortByName(e2, e3);

And with free functions, you don't have to do scope resolution (the "Employee::" bit). Any of these ways, except the first, should work with sort the same way, but I think the free functions would be the easiest route to implement.

Maybe your teacher wants you to do the first way, but that's really wrong.

Pick your poison.

Originally Posted by phantomotap

He is using, and implies the requirement for, `std::sort'.

I had forgotten that this was to be used for std::sort (or std::stable_sort). Given that sort just wants to compare two elements, like some others in this thread, I also don't see the reasoning behind using a class function to implement the compare function betwen two instances of classes to be used by std:sort. It's not like std::sort is a member function of the Person or Employee class, it's just a stand alone template function, so it seems that the compare function should also be a stand alone function.

Originally Posted by rcgldr

I had forgotten that this was to be used for std::sort (or std::stable_sort). Given that sort just wants to compare two elements, like some others in this thread, I also don't see the reasoning behind using a class function to implement the compare function betwen two instances of classes to be used by std:sort. It's not like std::sort is a member function of the Person or Employee class, it's just a stand alone template function, so it seems that the compare function should also be a stand alone function.

Since the majority of the users are suggesting that I declare the boolean functions outside of my Employee class it seems like I'm going to address this issue with my professor.

Originally Posted by laserlight

In this case, it looks like manasij7479's suggestion (refer to post #2) was already presented to you by your professor through the header file given.

I presume that you left out the header inclusion guards, headers included and using directive/declaration, and made a typo or two as whiteflags observed. Note the bad habits that your professor appears to have (or perhaps is just catering for you not having been taught them yet, but ugh):

• As mentioned earlier, you should not have a using directive or using declaration at file scope in a header file, so the various appearances of string should be std::string.
• std::string and Employee objects should be passed by (const) reference unless you really want to copy them.
• The sortByName and sortBySalary static member functions are really misnomers because they don't sort: they compare.

I always thought that std::string and string (enter variable name) were essentially the same thing. Until now, I never quite understood why people use std:: in headers as opposed to just regular string, cout, ...etc. When coding, I usually prefer the latter method probably because I learned Java before C++.

Originally Posted by rcgldr

If you had a general method to compare Persons, you could overload the < operator:

The less than operator that you wrote compares elements based on multiple criteria. Not really applicable in this case: the requirement is to sort by name or salary, not both. There is a difference.

Say you try to sort by salary first and then the name like this:

Code:

bool operator < (const Employee& a, const Employee& b)
{
  if (a.salary < b.salary) return true;
  if (a.name < b.name) return true;
  return false;
}

You end up with the sort going by salary and when two people being compared have the same salary (or b has a larger salary), then and only then are they sorted by name.

Originally Posted by FloatingButter

I always thought that std::string and string (enter variable name) were essentially the same thing.

They are, if in the given context string refers to std::string. The reason is that in this context, string belongs to the std namespace.

Originally Posted by FloatingButter

Until now, I never quite understood why people use std:: in headers as opposed to just regular string, cout, ...etc.

Because to use the unqualified names, you would need to have a using declaration (e.g., using std::string; ) or using directive (e.g., using namespace std; ). Unfortunately, a using declaration/directive at file scope in a header file will then affect the source files that include this header file. The source files may be written with the assumption that such a using declaration/directive is not present, thus this could result in an error or change the meaning of the code in an unintended manner.

Originally Posted by FloatingButter

When coding, I usually prefer the latter method probably because I learned Java before C++.

Java uses a package system rather than the somewhat more crude method of including header files. When in Rome, do as the Romans do.

Originally Posted by rcgldr

Class member functions are just code after all, and not associated with any actual instance of a class

In common C++ parlance, we call them "static member functions".

Originally Posted by rcgldr

If you had a general method to compare Persons, you could overload the < operator:

I think you have a bad habit of not being const-correct. Your example should be:

Code:

class Person {
public:
    // ...
    bool operator<(const Person& other) const;
    // ...
private:
    int age;
    std::string name;
};

bool Person::operator<(const Person &other) const {
    if (name < other.name) {
        return true;
    } else if (name > other.name) {
        return false;
    } else {
        return age < other.age;
    }
}

Notice the bool return type. I added the check for name > other.name, otherwise given a Person object x with age=1 and name="B" and another Person object y with age=2 and name="A", x < y == true and y < x == true, which violates the requirements for the comparator used by std::sort. Of course, in this case there does not appear to be any appropriate comparison in the task requirements for operator<.

O_o

@rcgldr: Look, I know you are trying to be helpful, but can you please stop posting answers without really considering your response. Sure, there is absolutely nothing wrong with trying to be helpful. I applaud that mindset even, but you have, essentially, pulled this same "Answer! Wait, no, Answer! Oh, nope $(Member) is right; it is Answer!" several times now. I don't buy even for a second that you knew what you were talking about and just briefly forgot. You are just still learning. There is no shame in that. You don't have to hide behind the internet. We don't care. There is nothing wrong with participation just because you are still learning. Nothing! Actually, most of us are only here to help people learn; the discussion is valuable precisely because of the educational possibilities. So please, stop pretending to the other newbies you are trying to help that you know what you are doing until you learn more yourself. Sure, go ahead and post the code; post your thoughts; post your suggestions, but at least note in consideration of others still trying to learn that you are still learning and may be wrong from the start.

It hasn't been so bad yet thanks mostly to laserlight being a machine, but at some point your going to send someone down the far wrong path with no one to back you up or cover your butt.

Also, deleting a post after a response has directly referenced it is really bad etiquette. I don't know you did delete it; I've seen my own posts vanish that I don't would have been removed by a moderator, but if you did, please don't do that.

[Edit]
Okay, I see why you deleted your post; so this last bit doesn't apply. Apologies.
[/Edit]

Soma

My last post got fouled up but IE8 got hung up and I couldn't get back. laserlights previous message is the same as what I ended up with, so use his example code for the < operator overload. Other than pasting to notepad or other editor, is there some way to keep stuff inside code tags from being reformatted, at least with IE8, the white space keeps getting messed up (spaces, newlines, ...) with each preview that I do, so I tried posting and editting, but obviously that's making things even worse.

@phanomotap - sorry, from now on, I'll check any code example I post by actually compiling and testing it before posting here. If IE8 is an issue for this forum, I can switch to google chrome.