Take the 2-minute tour ×
Code Review Stack Exchange is a question and answer site for peer programmer code reviews. It's 100% free, no registration required.

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.

share|improve this question
1  
Why can't you use sort method? –  Bruno Costa May 1 at 15:00
    
I can, I just wanted to see if I could do it without? –  BobbleHead May 1 at 15:01
1  
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. –  Bruno Costa May 1 at 15:28
1  
Actually I am sorry for that. I wasn't expecting a descending sort –  Bruno Costa May 1 at 15:43
2  
@BrunoCosta Nobody expects a descending sort. :-) –  svick May 1 at 16:16

5 Answers 5

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)\$.

share|improve this answer
    
Finding max can also be O(1) on a heap but that comes at a cost of O(lg n) in insertion –  Bruno Costa May 1 at 15:55
1  
@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. –  svick May 1 at 16:12
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...

share|improve this answer
1  
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. –  BobbleHead May 1 at 15:45

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)
{
share|improve this answer

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);
}
share|improve this answer
1  
booooo modifying the input parameters booooo –  insta May 1 at 16:09
    
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. –  Bruno Costa May 1 at 16:11
    
I didn't downvote, I'm just being a peanut gallery –  insta May 1 at 16:13

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.

share|improve this answer

Your Answer

 
discard

By posting your answer, you agree to the privacy policy and terms of service.

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