2
\$\begingroup\$

I had a problem where I needed to find the largest amount of passengers in multiple cars/vans etc, so I got the totals from each vehicle and added them to a list, I then created this method to sort the list...

static List<int> SortList(List<int> tobesorted)
{
    var j = 0;
    int JP = 0;

    int value1 = Convert.ToInt32(tobesorted[j]);
    int value2 = Convert.ToInt32(tobesorted[JP]);

    for (var i = 0; i < tobesorted.Count-1; i++)
    {
        for (j = 0; j < tobesorted.Count-1; j++)
        {
            JP = (j + 1);
            value1 = Convert.ToInt32(tobesorted[j]);
            value2 = Convert.ToInt32(tobesorted[JP]);

            if (value2 > value1)
            {
                tobesorted[j] = value2;
                tobesorted[JP] = value1;
            }
        }
        j = 0;
        JP = 0;
    }

    return tobesorted;
}

I don't understand is this is acceptable, and if not what methods are? It seems quick enough, I know of sorting algorithms, but haven't ever been forced to use any.

\$\endgroup\$
6
  • 1
    \$\begingroup\$ Why can't you use sort method? \$\endgroup\$ May 1, 2014 at 15:00
  • \$\begingroup\$ I can, I just wanted to see if I could do it without? \$\endgroup\$
    – BobbleHead
    May 1, 2014 at 15:01
  • 1
    \$\begingroup\$ I downvoted because I think you COULD put a little more effort in your code/research. Your sort algorithm as is do not sort. CodeReview NORMALY is a place where you provide code that SHOULD be semanthicaly correct or at least as correct as possible. \$\endgroup\$ May 1, 2014 at 15:28
  • 1
    \$\begingroup\$ Actually I am sorry for that. I wasn't expecting a descending sort \$\endgroup\$ May 1, 2014 at 15:43
  • 2
    \$\begingroup\$ @BrunoCosta Nobody expects a descending sort. :-) \$\endgroup\$
    – svick
    May 1, 2014 at 16:16

5 Answers 5

11
\$\begingroup\$

I needed to find the largest amount of passengers

If you need just the largest amount, you don't need to sort the whole list (which is slow*), you can instead walk though the list, remember the largest amount found so far and update that as you iterate the list.

LINQ already contains a method that does just that, called Max().


* Sorting is \$\mathcal{O}(n \log n)\$ with a good algorithm, or \$\mathcal{O}(n^2)\$ with a trivial algorithm like the one you used; finding maximum is just \$\mathcal{O}(n)\$.

\$\endgroup\$
2
  • \$\begingroup\$ Finding max can also be O(1) on a heap but that comes at a cost of O(lg n) in insertion \$\endgroup\$ May 1, 2014 at 15:55
  • 1
    \$\begingroup\$ @BrunoCosta Yes, a heap makes sense if you need to find max repeatedly in a changing collection, but I don't think that's the case here. \$\endgroup\$
    – svick
    May 1, 2014 at 16:12
6
\$\begingroup\$
int value1 = Convert.ToInt32(tobesorted[j]);
int value2 = Convert.ToInt32(tobesorted[JP]);

tobesorted is a List<int>. Hence, all its items are int. int is a C# language alias / shortcut for System.Int32 - therefore, the calls to Convert.ToInt32 are redundant and can be safely removed.

Who's JP? (i.e. use meaningful variable names ;)

List<T> already has a built-in Sort method that should do the trick...

\$\endgroup\$
1
  • 1
    \$\begingroup\$ Convert.ToInt32 is a bad habit of mine, I do it all over my code so I will take note of that. I think that's a remnant of the strings I was originally dealing with! Have taken them out though, so thank you. \$\endgroup\$
    – BobbleHead
    May 1, 2014 at 15:45
2
\$\begingroup\$

j is initialized every time the inner loop executes. So there is no need to define it in the outer scope. You can remove j in the outer scope and define the inner loop as

for (int j = 0; j < tobesorted.Count-1; j++)
    {
       // ...
    }

With this change there is no need to reset the value of j after the completion of inner loop.

Since tobesorted is a reference type so the method definition can be changed to

static void SortList(List<int> tobesorted)
{
\$\endgroup\$
2
\$\begingroup\$

Mat's Mug is completly right. You shouldn't be calling Convert.ToInt32 in your method since you already know that your list items are of type int. Here is your sort alghorithm, the selecting sort algorithm.

static List<int> SortList(List<int> list)
{
    for (int i = 0; i < list.Count-1; i++)
    {
        int idxMin = i;
        for (int j = i+1; j < list.Count; j++)
        {
            if (list[j] < list[idxMin])
            {
               idxMin = j;
            }
        }
        int aux = list[i];
        list[i] = list[idxMin];
        list[idxMin] = aux;
    }

    return list;
}

and a test case

List<int> list = new List<int>(){17,7,3,4,11,15,23,5,10,1};
foreach(int i in SortList(list)){
    Console.Write("{0}, ", i);
}
\$\endgroup\$
3
  • 1
    \$\begingroup\$ booooo modifying the input parameters booooo \$\endgroup\$ May 1, 2014 at 16:09
  • \$\begingroup\$ it's nothing less than an option, I guess... If you know that the method does that then it's ok. Though I think I would also prefer to do a copy. \$\endgroup\$ May 1, 2014 at 16:11
  • \$\begingroup\$ I didn't downvote, I'm just being a peanut gallery \$\endgroup\$ May 1, 2014 at 16:13
2
\$\begingroup\$

It looks to me that you're trying to scratch your ear by reaching around your elbow. Assuming you have a class representing the Vehicle, something like this:

enum VehicleType
{
    None,
    Van,
    Sedan,
    HatchBack,
    PickUp
}
public class Vehicle
{
    public VehicleType Type = VehicleType.None;
    public int MaxPassengers = 0;
    public int NumPassengers = 0;
}

And a list of these vehicles:

List<Vehicle> vehicles = new List<Vehicle>()
{
    new Vehicle{Type =  VehicleType.Van, MaxPassengers = 8, NumPassengers = 5},
    new Vehicle{Type = VehicleType.Sedan, MaxPassengers = 5, NumPassengers = 3},
    new Vehicle{Type = VehicleType.PickUp,MaxPassengers = 3, NumPassengers = 1},
};

The highest number of passengers would be:

int MostPassengers = vehicles.Max(x => x.NumPassengers);

The vehicle list sorted by number of passengers would be something like this:

List<Vehicle> SortedByPassengers = vehicles.OrderBy(x => x.NumPassengers);

Every vehicle at the bottom of the list with the same number of passengers will be the highest number of passengers.

\$\endgroup\$

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.