Problems with overloading operator+ in C++
I'm trying to overload addition operator where it also sorts the two added things as it adds.
Code:
class PhoneBookEntry{ How would I be able to do the subtraction...should it make the array smaller each time an item is removed or after all of the items are removed then just shift everything down...would anyone be able to show a quick code.. Any help would be appreciated...thank you |
After a quick look: Is this working? What's the value of "elements" when you create the new PhoneBookEntry??
Code:
PhoneBook(): phoneEntry(new PhoneBookEntry[elements]), size(0), elements(1){} Code:
for(; i < size; i++, k++){ I think you can achieve all this with only one, simple, loop: Code:
for(i=0;i<temp.size;++i) { |
Quote:
On a side note, it looks like you're using a merge sort algorithm in operator + (or a significant segment of one). In this case your 2 sections are already sorted and you are merging them into temp. I like how that's done. Quote:
You could streamline the first loop a little though: Code:
while (i < size && j < rhs.size) temp.phoneEntry[k++] = http://www.sgi.com/tech/stl/table_of_contents.html Can you show the insert function? Do you delete[], new[], and copy when the size of the list changes? Try the elements thing we suggested, and if that doesn't work, post the insert code. Thanks. ta0kira PS Please disable the smilies so that they don't show up in your code. Thanks. |
First off, that elements variable is garbage (never initialized) so it may have any value for the length of the phonebook entries array (as our friend mentioned). That is a REALLY bad thing as any value could be placed in there (including ZERO!!!) and your compiler may not even catch this.
I think you should consider changing the data when adding entries to a 'Phonebook' and grow the array appropriately based on the combined size. I see you have a method listed to do this, but no definition. THIS IS VERY IMPORTANT SINCE YOUR ADD METHOD MAY ADD PAST THE ALLOCATED SPACE FOR PhoneBookEntry. YOU MUST MALLOC MORE SPACE! IF YOU DO NOT YOU *CAN* WRITE TO INVALID MEMORY SPACE CAUSING AN EXCEPTION. Data structure books are a lot of help on this stuff. Also, making data public *could* be a bad idea. This is like a global variable. You might use accessor methods, that way you can check to make sure the data is valid (e.g. not zero length strings, make sure a phone_number really contains only numbers and dashes, etc). If this detection is not important, I guess it is not really a big issue. But this obviates the need for a class and data hiding (could just use struct). Anyhow here is some sample code (it assumes public members): Code:
//************************************************** Code assumes NULL is defined and that the < operator is overloaded in the PhoneEntries class (which it is not, yet). I suggest doing so because as far as my string implementations I have linked against, none can compare the entire string with just the < operator. But you may have such a library. It is correct to not assume others do, howerver and I suggest implementing a compare algorithm for your PhoneBook entry class. This is simply done by comparing each character in each string (of the name field, I assume). Code:
// NOT CODE, algorithm |
I don't think returning dynamic objects is appropriate in this case. I think the way operator + is written is just fine for OP's purpose (except 1 line; see below a little). As far as resizing phoneEntry; that is a good observation about needing to resize phoneEntry in temp, however this should be executed from temp. I'd suggest adding an unsigned int argument to the default constructor:
Code:
PhoneBook(unsigned int sSize = 1): elements(sSize), size(0), Then in operator + you would do this: Code:
PhoneBook temp; So; you really only need to do 4 small things (aside from individual style preferences) to make your code work (from what I see anyway): 1) make sure elements comes before phoneEntry in the initialization list 2) add an argument to your constructor that tells what size it should be to start with 3) make your size and elements variables unsigned 4) add a constructor argument when creating temp On another note; I would make the following change (in operator =), even though it probably won't cause errors in this case: Code:
phoneEntry = new PhoneBookEntry[rhs.size]; |
Just a friendly reminder that 'new' is a dynamic allocated object, just the same as malloc. malloc is the 'C' style, 'new' the C++ style. The same side effects and object scope apply.
Also, for large object data, may I suggest perhaps it is more appropriate to return a pointer to the object instead of the actual object (the stack could get HUGE for large PhoneBooks), in the worst case. Just a thought. Also a correction/note to add to my example code.... This code assumes that if 2 entries have the same name, they are the same entry!!! This may not be appropriate for your class. You may need to add both entries and change the indexes to suite your needs. Code:
// If not entry2 is either greater or equal to entry1 |
Quote:
I'll stick to my argument that the content of OP's operator + is suitable for his/her purpose. I think there's been some misunderstanding as to what's going on in the function. Please see my first post. ta0kira |
Well elements is initialized to 1, it allows me to know when the current array is full, so when size = elements, then I call the resize function and elements is increased...anyhow, here is a little refined code which also crashes...
Code:
PhoneBook PhoneBook::operator +(const PhoneBook& rhs) const{ Code:
phoneEntry[indexA].name {0x00321288 "John Doe"} std::basic_string<char,std::char_traits<char>,std::allocator<char> > Code:
else{ [code] static int __cdecl compare(const _Elem *_First1, const _Elem *_First2, size_t _Count) { // compare [_First1, _First1 + _Count) with [_First2, ...) return (::memcmp(_First1, _First2, _Count)); } [code] So, as you see it inserts the entry with the smaller letter but then when its about to add the other entry, it just cashes and I don't understand the reason for it...does anyone see the problem with this code? I appreciate other suggestions but I would like to be able to find the error in the code I have...thank you once again... |
[RESPONDING TO POST BEFORE LAST] Yes, it looks like the logic *looks* correct on the + operator. I just thought the usage of the for loops was a little unconventional what with all the 3 indexed variables, all with different control logic. That's a little difficult to read (if I turned that in to my engineering professor, I'd have to rewrite and consolidate). Not to mention difficult for someone else to debug (if not the author). Sorry if it sounded like I was being picky. I guess after being graded 100+ times on such things, I develop a need to consolidate and make the code more conventional (to most). As they tell our peers: "Just because it 'works', it does not mean it is well written code... Readability and style are factors...[comments too!]". But that is subjective anyhow ;). I have no need to enforce a certain style on anyone else, and I am sorry if it seemed this way. Also, is the '<' operator overloaded for strings?! I am not sure if it is, but the code assumes so. I do not remember seeing this in C++'s standard string library. I could be 100% wrong though.. not sure. I just know every time I have done a comparison, I needed to write the algorithm myself, char for char. As for the stack thing I mentioned, it won't be a problem for 'smaller' PhoneBooks. I am just considering the "worst case". To each their own, I appreciate the feedback. |
nevermind, I got it
|
Hi again,
I think I know... Here is the problem: Code:
PhoneBook temp; |
Quote:
elyk1212, yes that's what was wrong...so I just deleted the second line and made a the temp a dynamic for time being...thanks for catching it though =) Now, I have to do the samething but for the -, any suggestions on how to make do it the easiest way? Thanks for everything! |
That is going to be a pain, but not too bad ;). You will have to find every element NOT in PhoneBook you are subtracting and make a new PhoneElements array with just the ones left over. If none are found that match the records you are subtracting, it will do nothing (but naturally, and do not make this a special case).
Also, I think your '<' operator may only be comparing pointers for those string objects... not really sure though, it all depends on the 'default' usage of that operator. You better verify that string in fact implements this operator to do what you think it does. C++ can do interesting things when you are not explicit and it is hard to say what this operator is doing if it is not overloaded. A good example is, logical operators evaluate to true if there is an integer other than 0 as an argument. That might be unexpected.. .so for instance this would evaluate true, even if it were a boolean *kind of* object, with a false value: Code:
Pointer* some_pointer = &some_object; Or how about?: Code:
Pointer* some_pointer = &some_object; |
Quote:
|
All times are GMT -5. The time now is 08:19 PM. |