dynamic has this tendency of making everything that touches it
dynamic as well, which pretty much nullifies type safety and all its benefits: you want to cage that monster as early as possible, and avoid leaking it everywhere.
MaterialRepository should expose an
IReadOnlyList<Material>, not a
List<dynamic>. First an
IReadOnlyList<T> because you don't want the client code to have the right to treat the query results as a list (think
Remove), but merely as something they can iterate over: an
IEnumerable<T> would work fine, but an
IReadOnlyList<T> is better because then the client code won't trigger any warnings when it tries to iterate it more than once - which is something you have no control over from the repository class. And since you're already doing
.ToList(), might as well expose it as such to your clients.
What you're missing is a proper abstraction for your materials; what if you eventually need more than just
Value? With a
Material class, you could do this:
.Select(item => new Material
Code = item.Code.ToString(),
Value = item.Value.ToString()
And then you no longer need that
ComboBoxItem class - instead you can have
Material instances in your combo box! ..and if you have more than just a
Code and a
Value to display, you can get all that information at once, and reuse it.
Also you should consider using a
BindingList to data-bind your items instead of manually adding them:
var materialsBindingList = new BindingList(materials);
comboBox1.DataSource = materialsBindingList;
comboBox1.ValueMember = "Value";
comboBox1.DisplayMember = "Code";
That would pave the way to more modern UI frameworks, like Windows Presentation Foundation (WPF), where data binding is your go-to way of feeding data into a UI.
comboBox1 needs a name. It contains a material that the user selected.
SelectedMaterial seems like a much more meaningful name for it.
In this snippet:
var mRep= new MaterialRepository();
foreach (var item in listMaterial)
It's not clear where
listMaterial is coming from, and the coupling between what appears to be your UI (/form) and that
MaterialRepository class isn't ideal either. Why instantiate a new
MaterialRepository every time you need to call into it?
WinForms works well with a Model-View-Presenter pattern; you make a class to hold your "model" (exposing the available materials, user selections, etc.), and then the "view" uses it to get its data and store the state of things; the "presenter" coordinates everything, populates the "model", feeds it to the "view" and knows how and when to display and tear down the UI.
Refactoring toward MVP would help decouple your components and help with separating the application logic from the data access and UI components.