Sorting string array

Code:

---

        for (int i = 0; i < (BOOK_MAX); ++i)
        {
          Sort[i] = Books[i].Author;
        }
        sort(Sort, Sort+BOOK_MAX);
        cout << Sort[1] << Sort[2] << Sort[3] << endl;

---

I don't receive any errors at compile, but cout produces lots of empty spaces for some reason without the text I want. I tested with cout and Books[i].Author is not empty.

So, how many books do you actually have, and what is BOOK_MAX?

An empty string is "smaller" than all other strings, so if you have

Code:

---

"Bjarne Stroustrup"
"Scott Meyers"
"Barbara Moo"
""
""
""
""

---

then the sorted list would be:

Code:

---

""
""
""
""
"Barbara Moo"
"Bjarne Stroustrup"
"Scott Meyers"

---

--
Mats

BOOK_MAX is 2000, I have 28 books. However, I don't get any lines of text at all and the spaces don't look anywhere close to 2000.

I updated the code, and it still gives me lots of blanks.

Code:

---

for (int i = 0; i < (BOOK_MAX); ++i)
        {
          if (Books[i].Author == "")
            {
            }
          else
            {
          Sort[i] = Books[i].Author;
            }
        }
        sort(Sort, Sort+BOOK_MAX);
        cout << Sort[1] << Sort[2] << Sort[3] << endl;

---

Quote:

---

Originally Posted by Furious5k

BOOK_MAX is 2000, I have 28 books. However, I don't get any lines of text at all and the spaces don't look anywhere close to 2000.

I updated the code, and it still gives me lots of blanks.

Code:

---

for (int i = 0; i < (BOOK_MAX); ++i)
        {
          if (Books[i].Author == "")
            {
            }
          else
            {
          Sort[i] = Books[i].Author;
            }
        }
        sort(Sort, Sort+BOOK_MAX);
        cout << Sort[1] << Sort[2] << Sort[3] << endl;

---

---

So what does sort[i] contain if Books[i].Auther is ""?

I think you need two index variables, one for the sort array, and one for the books array, and then only increment the second index when the author is non-blank - then only sort the items you filled in [the second index variable will have that value].

--
Mats

I need to learn to read, I was still sorting the the whole of the array.

I am now trying to eliminate duplicates from the list before sorting it and after adding the code again getting blanks.

Code:

---

        int flag = 0;
        int count = 0;
        for (int i = 0; i < (BOOK_MAX); ++i)
          {
            if (Books[i].Author == "")
              {
              }
            else
              {
                for (int x = 0; x < count; ++x)
                  {
                    if (Books[i].Author == Sort[x])
                      {
                        ++flag;
                      }
                  }
                if (flag = 0)
                  {
                    Sort[count] = Books[i].Author;
                    ++count;
                  }
                flag = 0;
              }
          }
        sort(Sort, Sort+count);

---

It's probably better to sort the books first, then copy each unique author to another list - an author is unique if the previous author is different from the current one. After all, that only requires ONE iteration through the list of authors, not n^2 iterations like your current implementation.

--
Mats

It is much easier to remove duplicates after sorting. In an unsorted array the duplicate could be anywhere. In a sorted array the duplicates are all nicely next to each other.

In fact there is another algorithm for removing duplicates from a sorted container.

Code:

---

#include <iostream>
#include <algorithm>

int main()
{
    int arr[] = { 2, 1, 6, 4, 22, 4, 2, 1, 3 };
    const unsigned max = sizeof(arr) / sizeof(arr[0]);

    std::sort(arr, arr + max);

    //std::unique only works if the array is sorted
    const unsigned max_unique = std::unique(arr, arr + max) - arr;

    for (unsigned i = 0; i != max_unique; ++i) {
        std::cout << arr[i] << ' ';
    }

    //items in range max_unique...max are not valid
}

---

Code:

---

        int uniquecount;
        for (int x = 0; x < count; ++x)
          {
            if (Sort[x] == Sort[x+1])
              {
              }
            else
              {
                Unique[uniquecount] = Sort[x];
                ++uniquecount;
              }
          }

---

Urgh, segmentation fault. It seems perfectly fine and I'm sure it's something obvious, again...

Code:

---

Sort[x+1]

---

will access element number Count, which may not be defined, perhaps?

Or

Code:

---

        int uniquecount;

---

is never assigned a value, so it has some arbitrary (possibly LARGE) value.

--
Mats

uniquecount = 0; worked.

It still seems that the [x+1] would be accessing an array location that does not exist. But if it works, it doesn't matter, I guess, since it wouldn't cause wrong output.

Quote:

---

Originally Posted by Furious5k

uniquecount = 0; worked.

It still seems that the [x+1] would be accessing an array location that does not exist. But if it works, it doesn't matter, I guess, since it wouldn't cause wrong output.

---

Well, instead of looking at the next element, perhaps you should stuff the first element into the Unique array, and then compare with the latest element in the Unique array to see if you need to add it or not.

--
Mats

Code:

---

for (int q = 0; q < uniquecount; ++q)
          {
            for (int w = 0; w < BOOK_MAX; ++w)
              {
                if (Unique[q] == Books[w].Author)
                  {
                    cout << Books[w].Title << " in section " << Books[w].Category;

                    for (int e = 0; e < BOOK_MAX; ++e)
                      {
                        if (Book[w].ID == Loans[e].Book)
                          {
                            cout << " - on Loan." << endl;
                          }
                        else
                          {
                            cout << " - in Stock." << endl;
                          }
                      }
                  }
              }
          }

---

198: parse error before `['
213: break statement not within loop or switch
214: confused by earlier errors, bailing out

Line 198 is the if statement.
I'm confused about break statement, because the switch statement it's in is within braces.
The switch starts and ends as follows:

Code:

---

switch (select)
    {
    case 1:
      {

---

Code:

---

        break;
      }
    case 2:

---

I didn't have any errors before writing the code that starts at the point which I've pasted.

You probably have an error elsewhere - what you have posted seems OK to me.

--
Mats

I updated the code with everything up to the closing of the first bracket. It definately has to be something there, because if I wrap comment around that code it compiles.

How is Unique declared?

--
Mats

Code:

---

string Unique [BOOK_MAX];

---

So, can you post the whole function that this is in?

--
Mats

Uhh, all my code is int main. I'm still not very good with functions and due to deadlines I decided to just do it the way I can. Here's the whole case, if that helps any:

Code:

---

  int select;
  cin >> select;

  switch (select)
    {
    case 1:
      {
        int flag = 0;
        int count = 0;
        for (int i = 0; i < (BOOK_MAX); ++i)
          {
            if (Books[i].Author == "")
              {
              }
            else
              {
                    Sort[count] = Books[i].Author;
                    ++count;
              }
          }
        sort(Sort, Sort+count);

        int uniquecount = 0;
        for (int x = 0; x < count; ++x)
          {
            if (Sort[x] == Sort[x+1])
              {
              }
            else
              {
                Unique[uniquecount] = Sort[x];
                ++uniquecount;
              }
          }

        /*for (int q = 0; q < uniquecount; ++q)
          {
            for (int w = 0; w < BOOK_MAX; ++w)
              {
                if (Unique[q] == Books[w].Author)
                  {
                    cout << Books[w].Title << " in section " << Books[w].Category;

                    for (int e = 0; e < BOOK_MAX; ++e)
                      {
                        if (Book[w].ID == Loans[e].Book)
                          {
                            cout << " - on Loan." << endl;
                          }
                        else
                          {
                            cout << " - in Stock." << endl;
                          }
                      }
                  }
              }
          }
        */

        cout << Unique[1] << Unique[2] << endl;
        break;
      }

---

And about 5 lines around the declaration of Unique...

--
mats

Code:

---

Book Books [BOOK_MAX];
User Users [USER_MAX];
Loan Loans [BOOK_MAX];
string Sort [BOOK_MAX];
string Unique [BOOK_MAX];

int main()

---

Quote:

---

Originally Posted by Furious5k

Uhh, all my code is int main. I'm still not very good with functions and due to deadlines I decided to just do it the way I can.

---

I have a feeling that this is a case of "more haste less speed".

You could start by turning the block of code following case 1 into a function. Give it a descriptive name, add a comment that describes what the function is supposed to do. It looks like you made the mistake of using global variables unnecessarily, but nevermind, for the function practice using arguments. You would find that when you focus on a small piece of code without distractions (thanks to local scope), reasoning about your program and figuring out what is wrong becomes easier.

If you're allowed to use the standard library for sorting, perhaps you're also allowed to use it for removing duplicates as well?

Code:

---

                        if (Book[w].ID == Loans[e].Book)

---

see anything wrong? If not, look closer!

Code:

---

            if (Sort[x] == Sort[x+1])
              {
              }
            else
              {
                Unique[uniquecount] = Sort[x];
                ++uniquecount;
              }

---

How about using the != operator instead of == operator?

And x+1 still goes outside of the array.

Yes, I agree with Laserlight - your case 1: should definitely be it's own function.

--
Mats

Why don't you just overload operator< for your Books Class and call sort() directly on your class.

Code:

---

bool Books::operator<(const Books obj) const
{
    if(Author < obj.Author)
        return true;
    else if (Author == obj.Author)
        return true;
    else return false;
}

---

Then just delete the empty arrays. Of course I am usually wrong.

Quote:

---

Originally Posted by Phyxashun

Why don't you just overload operator< for your Books Class and call sort() directly on your class.

Code:

---

bool Books::operator<(const Books obj) const
{
    if(Author < obj.Author)
        return true;
    else if (Author == obj.Author)
        return true;
    else return false;
}

---

Then just delete the empty arrays. Of course I am usually wrong.

---

That's not a bad idea - but surely operator< should not return true for Author == obj.Author? [and const Books& obj, perhaps?]

--
Mats

Quote:

---

Originally Posted by matsp

That's not a bad idea - but surely operator< should not return true for Author == obj.Author? [and const Books& obj, perhaps?]

--
Mats

---

I agree with the passing by reference, but if you have books of the same author wouldn't they be equal? i'm not too sure.

EDIT:

Well wouldn't you want them to be treated equal by the sort() algorithm, you can also add nested ifs to the overload function to include the other members of the class so that the sort() would be more specific than just sorting the authors. Just me pondering.

Quote:

---

Originally Posted by Phyxashun

but if you have books of the same author wouldn't they be equal?

---

If they are "equal", then one is not "less than" the other, so you should return false.

Quote:

---

Originally Posted by laserlight

If they are "equal", then one is not "less than" the other, so you should return false.

---

Ah, more failed logic on my part. I agree ;)

EDIT:
And if you wanted to move the empty class members to the end couldn't you change the overloaded operator< to make it >

Code:

---

bool Books::operator<(const Book &obj) const
{
    if (Author > obj.Author)
        return true;
    return false;
}

---

Quote:

---

Originally Posted by Phyxashun

And if you wanted to move the empty class member to the end couldn't change to overloaded operator< to make it >

---

That would result in a reverse ordered sort. More accurately:

Code:

---

bool Books::operator<(const Book &obj) const
{
    if (Author.empty())
    {
        return !obj.Author.empty();
    }
    else if (obj.Author.empty())
    {
        return false;
    }
    return Author < obj.Author;
}

---

Quote:

---

Originally Posted by laserlight

That would result in a reverse ordered sort.

---

I like it! You know, I have been learning more through reading this board, and posting; than I have learned from any book that I have found over the years.

Thanks!

Eh, sorry ah, but I realised that I mixed up the order myself. I should have come up with the edge cases and tested first. A correct version that places empty strings at the end would be:

Code:

---

bool Books::operator<(const Book &obj) const
{
    if (obj.Author.empty())
    {
        return !Author.empty();
    }
    else if (Author.empty())
    {
        return false;
    }
    return Author < obj.Author;
}

---

EDIT:
Or slightly more succinctly:

Code:

---

bool Books::operator<(const Book &rhs) const
{
    if (rhs.Author.empty())
    {
        return !Author.empty();
    }
    return !Author.empty() && Author < rhs.Author;
}

---

It appears that the test that puts zero items last needs to be a bit more complicated:

Code:

---

#include <iostream>
#include <algorithm>
#include <iterator>
#include <cstdlib>

class X
{
    int n;
public:
    X(): n(rand()%4) {}
    bool operator<(X rhv) const
    {
        if (n == 0) {
            return false;
        }
        else if (rhv.n == 0) {
            return true;
        }

        else {
            return n < rhv.n;
        }
    }
    friend std::ostream& operator<<(std::ostream& os, X x)
    {
        return os << x.n;
    }
};

int main()
{
    const unsigned max = 80;
    X arr[max] = {};
    std::sort(arr, arr + max);
    std::copy(arr, arr + max, std::ostream_iterator<X>(std::cout, ""));
}

---

Edit: I see you've already noticed that...

I was going to also suggest that the books be a vector instead of an array. That would make the sorting easier, and the moving of the empty data to the end easier.

Would it not?

Quote:

---

Originally Posted by anon

I see you've already noticed that...

---

Ah, but your logic is slightly simpler. I should have tried to simplify the logic before trying to write the expression more succinctly. Consequently, we can more easily reduce the expression to:

Code:

---

bool Book::operator<(const Book& rhs) const
{
    return !Author.empty() && (rhs.Author.empty() || Author < rhs.Author);
}

---

Quote:

---

Originally Posted by Phyxashun

I was going to also suggest that the books be a vector instead of an array. That would make the sorting easier, and the moving of the empty data to the end easier.

Would it not?

---

It would not make the sorting simpler, but there are other advantages, like having a dynamic size and not needing to manually manage memory.