Implementing Search method For a Class

Question

In my web app, I have a view where users can search in receipts and drafts.

I've implemented this ability in search method at my view model, but I'm not sure if this is the best place to put it. So I'll let you take a look at it.

public class ProductCardSearchViewModel
{
    public IEnumerable<ProductCardRowViewModel> Search()
    {
        GenericRepository<Rows> repo = new GenericRepository<Rows>();
        var temp = repo.GetAll();

        if (this.Warehouse != 0)
            temp = temp.Where(x => x.Head.Warehouse == this.Warehouse );
        if (ProductCode > 0)
            temp = temp.Where(x => x.ProductCode == this.ProductCode );
        if (!string.IsNullOrEmpty(this.UnitCode))
            temp = temp.Where(x => x.UnitCode .Equals(this.UnitCode));

        List<ProductCardRowViewModel> data = new List<ProductCardRowViewModel>();

        foreach (Rows row in temp)
            data.Add(new ProductCardRowViewModel(row));

        return data;
    }

    [Display(Name = "")]
    public long Warehouse { get; set; }

    [Display(Name = "")]
    public string UnitCode { get; set; }

    [Display(Name = "")]
    public long ProductCode { get; set; }
}

Please feel free to suggest any improvements in this code. I'm not much interested to stick with design patterns, but I like to have SOLID code, and I'm kind of sure that this one is not!

I may be wrong, but I think this is mostly SOLID. The only reason I can see to change the code would be to change which fields you're searching, so that's the S. I don't see any inheritance, so the O doesn't apply. Same thing for the L. I don't see any monster interfaces, so the I doesn't apply. The one problem I see is that you new up a GenericRepository<Rows>. This violates the D. That dependency should be injected. — user2023861
– user2023861, Commented Oct 15, 2015 at 15:43

Heslacher · Accepted Answer · 2015-10-15 08:38:35Z

public IEnumerable<ProductCardRowViewModel> Search()
{
    GenericRepository<Rows> repo = new GenericRepository<Rows>();
    var temp = repo.GetAll();

    if (this.Warehouse != 0)
        temp = temp.Where(x => x.Head.Warehouse == this.Warehouse );
    if (ProductCode > 0)
        temp = temp.Where(x => x.ProductCode == this.ProductCode );
    if (!string.IsNullOrEmpty(this.UnitCode))
        temp = temp.Where(x => x.UnitCode .Equals(this.UnitCode));

    List<ProductCardRowViewModel> data = new List<ProductCardRowViewModel>();

    foreach (Rows row in temp)
        data.Add(new ProductCardRowViewModel(row));

    return data;
}

omitting braces {} in places where they are optional is bad practice and leads to error prone code.
in its current state this method is returning for Warehouse == 0, ProductCode == 0 and for instance UnitCode == string.Empty all items. If this is what you want then IMO a comment should describe this behaviour.
you are sometimes using this and sometimes you don't. Choose one style and stick to it.
I would add a ToList() to temp in the loop. And by the way temp is a really bad name.
instead of building the returned value as a List<T> you could simply use yield return. If you want to keep the List you should at least use var.

Applying the mentioned points would lead to

public IEnumerable<ProductCardRowViewModel> Search()
{
    GenericRepository<Rows> repo = new GenericRepository<Rows>();
    var rows = repo.GetAll();

    if (this.Warehouse != 0)
    {
        rows = rows.Where(x => x.Head.Warehouse == this.Warehouse );
    }

    if (this.ProductCode > 0)
    {
        rows = rows.Where(x => x.ProductCode == this.ProductCode );
    }

    if (!string.IsNullOrEmpty(this.UnitCode))
    {
        rows = rows.Where(x => x.UnitCode.Equals(this.UnitCode));
    }

    foreach (Rows row in rows.ToList())
    {
        yield return new ProductCardRowViewModel(row);
    }
}

Thank you. There are just two things: 1. You think this method is in the right place, in a view model? 2. I didn't really get the rows.ToList() part. — Akbari
– Akbari, Commented Oct 15, 2015 at 6:56
If I see this correctly, it seems that your posted code is not complete. There isn't any constructor which is taking a Rows argument. I didn't notice this at first, otherwise I would have commented about this and would not have answered this. So right now in its current state I can't answer 1.. About the second point, if the GetAll() method returns an IEnumerable<T> which is fetched from a database then the call to ToList() will reduce the amounts of trips to the database. — Heslacher
– Heslacher, Commented Oct 15, 2015 at 7:15
Sorry, there is a little mistake in the posted code, it doesn't return a list of itself, but another class. This is kind of old code and I didn't notice this. I updated the post. — Akbari
– Akbari, Commented Oct 15, 2015 at 7:21

t3chb0t · Accepted Answer · 2015-10-15 15:25:21Z

I'd simplify you queries into a single Where like this one and throw away the loop and return a clean IEnumerable. I also think the this keyword is not necessary.

public IEnumerable<ProductCardRowViewModel> Search()
{
    var repo = new GenericRepository<Rows>();
    var rows = repo.GetAll();

    var result =
        rows
        .Where(x => 
            (Warehouse == 0 || x.Head.Warehouse == Warehouse) &&
            (ProductCode == 0 || x.ProductCode == ProductCode) &&
            (string.IsNullOrEmpty(UnitCode) || x.UnitCode.Equals(UnitCode))
        .Select(x => new ProductCardRowViewModel(row));

    return result;
}

Stack Exchange Network

Implementing Search method For a Class

2 Answers 2

You must log in to answer this question.

Hot Network Questions

Implementing Search method For a Class

2 Answers 2

You must log in to answer this question.

Related

Hot Network Questions