Skip to main content
deleted 18 characters in body
Source Link
Heslacher
  • 51k
  • 5
  • 83
  • 177
public IEnumerable<ProductCardSearchViewModel>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<ProductCardSearchViewModel>List<ProductCardRowViewModel> data = new List<ProductCardSearchViewModel>List<ProductCardRowViewModel>();

    foreach (Rows row in temp)
        data.Add(new ProductCardSearchViewModelProductCardRowViewModel(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<ProductCardSearchViewModel>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 ProductCardSearchViewModelProductCardRowViewModel(row);
    }
}
public IEnumerable<ProductCardSearchViewModel> 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<ProductCardSearchViewModel> data = new List<ProductCardSearchViewModel>();

    foreach (Rows row in temp)
        data.Add(new ProductCardSearchViewModel(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<ProductCardSearchViewModel> 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 ProductCardSearchViewModel(row);
    }
}
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);
    }
}
Source Link
Heslacher
  • 51k
  • 5
  • 83
  • 177

public IEnumerable<ProductCardSearchViewModel> 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<ProductCardSearchViewModel> data = new List<ProductCardSearchViewModel>();

    foreach (Rows row in temp)
        data.Add(new ProductCardSearchViewModel(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<ProductCardSearchViewModel> 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 ProductCardSearchViewModel(row);
    }
}