4
\$\begingroup\$

Here I'm merging a list to end of another list after sorting it according to another list.This should sort processdetails list according to the order in routeplans list if any item not present in processdetails list is found it should keep the order and add it to processdetails list. Simply saying it just merging two lists based on another list.The below code works fine, I want to know whether there is cleaner way of doing this.if possible in linq.

using System;
using System.Collections.Generic; 
using System.Linq;
using System.Text;
using System.Threading.Tasks;

namespace SortByList
{
public class OnGoingProcess
{
    public long Id { get; set; }

    public string Name { get; set; }
    public string Code { get; set; }
}

class Program
{
    static void Main(string[] args)
    {
        var processDetails = new List<OnGoingProcess>();
        processDetails.Add(new OnGoingProcess() { Id = 120, Name = "Process 29", Code = "T20" });
        processDetails.Add(new OnGoingProcess() { Id = 100, Name = "Process 2", Code = "A20" });
        processDetails.Add(new OnGoingProcess() { Id = 99, Name = "Process 3", Code = "S20"  });
        processDetails.Add(new OnGoingProcess() { Id = 85, Name = "Process 229", Code = "B20" });
        processDetails.Add(new OnGoingProcess() { Id = 220, Name = "Process 39", Code = "C20" });
        processDetails.Add(new OnGoingProcess() { Id = 15, Name = "Process 20", Code = "D20" });
        processDetails.Add(new OnGoingProcess() { Id = 101, Name = "Process 129", Code = "G20" });

        var routePlans = new List<OnGoingProcess>();
        routePlans.Add(new OnGoingProcess() { Id = 100, Name = "Process 2" });
        routePlans.Add(new OnGoingProcess() { Id = 120, Name = "Process 29" });
        routePlans.Add(new OnGoingProcess() { Id = 222, Name = "Process new" });
        routePlans.Add(new OnGoingProcess() { Id = 85, Name = "Process 229" });
        routePlans.Add(new OnGoingProcess() { Id = 101, Name = "Process 129" });

        var sortedPlans = new List<OnGoingProcess>();

        foreach (var plan in routePlans)
        {
            var selectedDetails = processDetails.Where(x => x.Id == plan.Id).ToList();

            if (selectedDetails.Any())
            {
                sortedPlans.AddRange(selectedDetails);
                processDetails.RemoveAll(x => x.Id == plan.Id);

            }
            else
            {
                sortedPlans.Add(plan);
            }
        }
        processDetails.AddRange(sortedPlans);

        foreach (var sorted in processDetails)
        {
            Console.WriteLine("Id : " + sorted.Id + " Name : " + sorted.Name + " Code : " + sorted.Code );
        }

        Console.ReadKey();

     //Id : 99 Name : Process 3 Code : S20
     //Id : 220 Name : Process 39 Code : C20
     //Id : 15 Name : Process 20 Code : D20
     //Id : 100 Name : Process 2 Code : A20
     //Id : 120 Name : Process 29 Code : T20
     //Id : 222 Name : Process new Code :
     //Id : 85 Name : Process 229 Code : B20
     //Id : 101 Name : Process 129 Code : G20

    }
}
}
\$\endgroup\$
  • 1
    \$\begingroup\$ Can you add some context and motivation please. \$\endgroup\$ – πάντα ῥεῖ Jan 2 '17 at 12:51
  • \$\begingroup\$ This for planning process, The process route may change according to different products, So the order may change. \$\endgroup\$ – Manioth Shijith Jan 2 '17 at 13:10
  • \$\begingroup\$ Please edit the context into the question. \$\endgroup\$ – Heslacher Jan 2 '17 at 13:12
  • \$\begingroup\$ @ManiothShijith Edit your question please. Code dump only questions will not be well achieved. See also How do I ask a good question? please. \$\endgroup\$ – πάντα ῥεῖ Jan 2 '17 at 13:13
  • \$\begingroup\$ This makes no sense to me "if any item not present in processdetails list is found it should keep the order and add it to processdetails list" \$\endgroup\$ – paparazzo Jan 2 '17 at 15:15
2
\$\begingroup\$

Very poor problem statement. Problem statement is based on your perceived solution so review is difficult. I could only figure out what you are trying to do by reviewing the code.

Why long Id? If you have that many OnGoingProcess then will probably need a different approach.

Have proper constructors for OnGoingProcess and make Id read only.

Create a proper application and test. Don't just jam everything in Main.

You are changing selectedDetails. A calling procedure does not want an input changed. Output a new List mergedDetails.

Proper problem statement
processDetails and sortedPlans are each List of OnGoingProcess identified by property Id
Need output in this order

  • processDetails not present in sortedPlans (in the order in processDetails)

  • sortedPlans (in the order of sortedPlans)
    if present in processDetails then processDetails
    else then sorted plan

Putting those thoughts in action

public static void OnGoingProcessMergeTest()
{
    var processDetails = new List<OnGoingProcess>()
    { new OnGoingProcess(120, "Process 29", "T20"), new OnGoingProcess(100, "Process 2", "A20"), new OnGoingProcess(99, "Process 3", "S20"),
      new OnGoingProcess(85, "Process 229", "B20"), new OnGoingProcess(220, "Process 39", "C20"), new OnGoingProcess(15, "Process 20", "D20"),
      new OnGoingProcess(101, "Process 129", "G20")};

    var routePlans = new List<OnGoingProcess>()
    { new OnGoingProcess(100, "Process 2"), new OnGoingProcess(120,"Process 29"), new OnGoingProcess(222, "Process new"),
      new OnGoingProcess(85, "Process 229"), new OnGoingProcess(101, "Process 129")};

    List<OnGoingProcess> mergedDetails = OnGoingProcessMerge2(processDetails, routePlans);
    foreach (var sorted in mergedDetails)
    {
        Debug.WriteLine("Id : " + sorted.Id + " Name : " + sorted.Name + " Code : " + sorted.Code);
    }
    Debug.WriteLine("");
}       
public static List<OnGoingProcess> OnGoingProcessMerge2(List<OnGoingProcess> processDetails, List<OnGoingProcess> routePlans)
{
    List<OnGoingProcess> onGoingProcessMerge = processDetails.Where(x => !(routePlans.Any(y => y.Id == x.Id))).ToList();
    foreach(OnGoingProcess routePlan in routePlans)
    {
        OnGoingProcess processDetail = processDetails.FirstOrDefault(x => x.Id == routePlan.Id);
        onGoingProcessMerge.Add(processDetail ?? routePlan);
    }
    return onGoingProcessMerge;
}
\$\endgroup\$
  • 1
    \$\begingroup\$ You can simplify to onGoingProcessMerge.Add(processDetail ?? routePlan); rather than the if/else at the end. \$\endgroup\$ – RobH Jan 3 '17 at 11:39
1
\$\begingroup\$

You can definitely use the collection initializer showed by @Paparazzi

var processDetails = new List<OnGoingProcess>()
{ 
    new OnGoingProcess(120, "Process 29", "T20"), 
    new OnGoingProcess(85, "Process 229")
};

Like others I have difficulties to understand the requirements but I think what you are looking for is a Union

Produces the set union of two sequences by using a specified IEqualityComparer.

With it you can replace the entire foreach as

var mergedProcesses = processDetails.Union(routePlans, new ProcessEqualityComparer());

where

class ProcessEqualityComparer : IEqualityComparer<OnGoingProcess>
{
    public bool Equals(OnGoingProcess left, OnGoingProcess right) 
    {
        return left.Id == right.Id;
    }

    public int GetHashCode(OnGoingProcess process)
    {
        return process.Id.GetHashCode();
    }
}

The result is:

120 Process 29  T20
100 Process 2   A20
99  Process 3   S20
85  Process 229 B20
220 Process 39  C20
15  Process 20  D20
101 Process 129 G20
222 Process new null
\$\endgroup\$
  • \$\begingroup\$ This does't keep the order, The output I got running this sample is 'code' 'Id : 120 Name : Process 29 Code : T20 Id : 100 Name : Process 2 Code : A20 Id : 99 Name : Process 3 Code : S20 Id : 85 Name : Process 229 Code : B20 Id : 220 Name : Process 39 Code : C20 Id : 15 Name : Process 20 Code : D20 Id : 101 Name : Process 129 Code : G20 Id : 222 Name : Process new Code :' "Process new Code" moved to last. \$\endgroup\$ – Manioth Shijith Jan 3 '17 at 9:18
  • \$\begingroup\$ It should keep the order \$\endgroup\$ – Manioth Shijith Jan 3 '17 at 9:20
  • \$\begingroup\$ @ManiothShijith you're right, I've pasted the wrong result. Yes, it changes the order. To keep it there're should be property that would allow sorting them like a SequenceNumber or a Timestamp or something. What's with the order? Why do you need to maintain it? There doesn't seem to be a reason for it. Just curious. \$\endgroup\$ – t3chb0t Jan 3 '17 at 9:32
  • \$\begingroup\$ This will be used to plan some processes for creating a product, The processes are different for different products, There may be situation where the user has to change order of some processes(Different Plan) so they will pass the order using route plans list, So its important to maintain the order of list. \$\endgroup\$ – Manioth Shijith Jan 3 '17 at 9:43
  • \$\begingroup\$ @ManiothShijith I'm afraid there might not be a better way for solving this without loosing the order. The merge is very specific. \$\endgroup\$ – t3chb0t Jan 3 '17 at 10:02
0
\$\begingroup\$

I want to know whether there is cleaner way of doing this

I would say that writing it object oriented will do that.

Improvements:

  • The code is simpler everywhere, because:
    • OnGoingProcess overrides puts that functionality where it belongs - Single Responsibility principle
    • Built-in .NET functionality uses those overrides; less code for us to write.
  • main is far easier to understand
  • OnGoingProcess is re-usable
  • OnGoingProcessCollection is simple because OnGoingProcess takes responsibility for knowing how to compare & equate itself.
  • LINQ is simplified by using List methods that take predicates.
  • OnGoingProcess.CompareTo
    • makes List.Sort work
    • simplifies LINQ statements
  • OnGoingProcess.Equals
    • makes List.Contains work
    • simplifies LINQ statements
  • ICompareable<T>, not ICompareable
    • OnGoingProcess.CompareTo is strongly typed. No need to test the parameter type for as OnGoingProcess
  • LINQ and Object Oriented Programming
    • LINQ is quick and concise. I think this is why we tend to forget about OO when we use it.
    • Client code should not, for example, be telling an object how that class defines equality. And it is all to tempting to simply use Orderby rather than impliment IComparable.
    • LINQ should be used to augment .NET collection functionality, not replace it.

.

using System;
using System.Collections.Generic;
using System.Linq;
using System.Text;
using System.Threading.Tasks;

namespace SortAndMerge
{   

        class Program
        {
            static void Main( string[] args )
            {
                var processDetails = new OnGoingProcessCollection();
                processDetails.Add( new OnGoingProcess(100, "Process 2",  "A20"  ));
                processDetails.Add( new OnGoingProcess(120, "Process 29", "T20"  ));
                processDetails.Add( new OnGoingProcess(99 , "Process 3",  "S20"  ));
                processDetails.Add( new OnGoingProcess(85 , "Process 229",  "B20")  );
                processDetails.Add( new OnGoingProcess(220, "Process 39",  "C20" ) );
                processDetails.Add( new OnGoingProcess(15 , "Process 20",  "D20" ) );
                processDetails.Add( new OnGoingProcess(101, "Process 129",  "G20")  );

                var routePlans = new OnGoingProcessCollection();
                routePlans.Add( new OnGoingProcess(100, "Process 2" , null ));
                routePlans.Add( new OnGoingProcess(120, "Process 29" , null) );
                routePlans.Add( new OnGoingProcess(222, "Process new" ,null) );
                routePlans.Add( new OnGoingProcess(85, "Process 229" , null) );
                routePlans.Add( new OnGoingProcess(101, "Process 129" ,null) );

                processDetails.MergeUnique( routePlans );
                Console.WriteLine( processDetails );

                Console.WriteLine("\n\nSorted:");
                processDetails.Sort();
                Console.WriteLine(processDetails);

                Console.ReadKey();    
            }
        }


        public class OnGoingProcess : IComparable<OnGoingProcess>
        {
            public int Id      { get; protected set; }
            public string Name { get; protected set; }
            public string Code { get; protected set; }

            public OnGoingProcess( int id, string name, string code )
            {
                Id = id;
                Name = name ?? string.Empty;
                Code = code ?? string.Empty;
            }

            public int CompareTo( OnGoingProcess other )
            {
                if ( other == null )  return 1;

                return Id.CompareTo( other.Id );
            }

            public override bool Equals( object obj )
            {
                OnGoingProcess temp = obj as OnGoingProcess;

                if ( temp == null )  return false;

                return this.Id == temp.Id;
            }

            public override int GetHashCode()
            {
                return Id.GetHashCode();
            }
        }


        class OnGoingProcessCollection : List<OnGoingProcess>
        {

            public void MergeUnique( OnGoingProcessCollection target )
            {
                this.AddRange( ( target.FindAll( x => !this.Contains( x ) ).AsEnumerable() ) );
            }


            public override string ToString()
            {
                StringBuilder me = new StringBuilder();
                this.ForEach( x =>  me.AppendFormat( " Id : {0}  Name : {1}  Code : {2}\n", x.Id, x.Name, x.Code )); 
                return me.ToString();
            }
        }

}
\$\endgroup\$
  • \$\begingroup\$ The order is not defined. Otherwise anyone could simply sort it by Id. Implementing the IComparable for this is kind of overkill. \$\endgroup\$ – t3chb0t Jan 3 '17 at 16:59
  • \$\begingroup\$ The title is "Sort and Merge..." and objects are filtered by Id, but the code does not do any sorting at all! So I deduced it to be by Id. I further deduced that the class design is intended to sorted by id (at least by default). ... "anyone could sort by id .... IComparable is overkill". A well designed class hides state and exposes functionality. To the extent that a class is intended to be used in a certain way that "way" must be in that class. And doing so embiggens all the other SOLID and OO goodness in any given design. \$\endgroup\$ – radarbob Jan 3 '17 at 17:42
  • \$\begingroup\$ There is actually sorting, Its sorting the items( if present in processdetails list) according to the order of route plan list. \$\endgroup\$ – Manioth Shijith Jan 4 '17 at 8:02
  • \$\begingroup\$ As far as I can tell, there is no sorting. The final state of processDetails appears to be simply a consequence of the removal of duplicate, and insertion of missing, processes in "iteration order." If the list is being sorted it is well hidden. This obfuscation is what my code tries to address. CompareTo defines sort order and sorting is a distinct method call. \$\endgroup\$ – radarbob Jan 4 '17 at 13:21

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.