Problem
Introduction
I want to create an Inventory App in C#. I was thinking of someone who sells a few products and only needs to have a few product properties in the program to search or sort by, and then click on ‘sold’ or click on ‘order’. An Excel export is also to be realised.
Today, I wrote my search function. The search term is entered in a text box. One can enclose the search term in inverted commas “to find exactly this whole sentence”; one can also write A Or B to find both products; one can also write A -B to find A, but not B. So with the latter example, you can exclude certain words.
There is a list of entries in my programme. It contains instances of the class Entry. The class Entry also includes an instance of the class Product.
At the end of the search function, however, another list – search_results – is filled. This is sorted.
My question
I would like to know if there is anything I can do to improve this. It seems like a lot of lines of code to me…
Edit: This is a Windows Forms App (.NET Framework) with .Net Framework 4.8
FormMain.cs
/// <summary>
/// all Data
/// </summary>
public static readonly List<Entry> AllEntries = new List<Entry>();
/// <summary>
/// Suchergebnisse
/// </summary>
private List<Entry> search_results = null;
private enum Esort_criterion
{
alphabetical,
by_Creation_Time
}
private Esort_criterion Sort_Criterion;
private void TextBox_seek_KeyUp(object sender, KeyEventArgs e)
{
if (e.KeyCode == Keys.Enter && !System.String.IsNullOrEmpty(TextBox_seek.Text))
{
seek(TextBox_seek.Text);
}
}
private async void seek(string SearchTerm)
{
// Remove entries from the previous search (if there are any).
if (search_results != null && search_results.Count > 0)
{
foreach (Entry E in search_results)
{
foreach (Bitmap Image in E.TheProduct.ListOfPhotos)
{
if (Image != null)
{
Image.Dispose();
}
}
}
search_results.Clear();
}
List<Entry> sorted_search_results = await Task.Run(() => seekAsync(SearchTerm));
if (sorted_search_results == null)
{
return;
}
if (sorted_search_results.Count == 0)
{
MessageBox.Show("no matches found",
"Inventory App – Suche",
MessageBoxButtons.OK,
MessageBoxIcon.Information);// Do not write ‘return’
}
ListBox1.Items.Clear();
ListBox1.Items.AddRange(search_results.Select(x => $"{x.TheProduct.BrandName}{"t"}{x.TheProduct.ModelName}{"t"}{x.TheProduct.DescriptionOrNotes}{"t"}{Math.Round(x.TheProduct.SellingPrice, 2)} €").ToArray());
}
private List<Entry> seekAsync(string SearchTerm)
{
search_results = new List<Entry>();
char[] CharArray = SearchTerm.ToCharArray();
string[] StringArray1 = SearchTerm.Split(new[] { " Or ", " or " }, 3, StringSplitOptions.None);
string[] StringArray2 = SearchTerm.Split(new[] { " -" }, 3, StringSplitOptions.None);
// "Mehrere Wörter zusammenhängend" (exaktes Ergebnis)
if (CharArray.First() == '"' && CharArray.Last() == '"')
{
foreach (Entry E in AllEntries)
{
if (E.TheProduct.BrandName.Contains(SearchTerm.Split('"')[1]) ||
E.TheProduct.ModelName.Contains(SearchTerm.Split('"')[1]))
{
List<Bitmap> NewBitmaps = new List<Bitmap>();
foreach (string FullFileName in E.TheProduct.ListOfPathsOfThePhotos)
{
if (System.IO.File.Exists(FullFileName))
{
NewBitmaps.Add(new Bitmap(FullFileName));
}
}
// NEUE Instanzen erzeugen, sonst wird auf denselben Speicherbereich gezeigt.
// Create NEW instances, otherwise the same memory area is pointed to.
search_results.Add(new Entry(
E.Employee_who_created_Entry,
E.CreationTime,
E.Changed_by_Employee,
E.LastChangeTime,
new Product(
E.TheProduct.BrandName,
E.TheProduct.ModelName,
E.TheProduct.Size,
E.TheProduct.DescriptionOrNotes,
NewBitmaps,
E.TheProduct.ListOfPathsOfThePhotos,
E.TheProduct.PurchasePrice,
E.TheProduct.SellingPrice)
)
);
search_results.Last().TheProduct.are_images_loaded = (new bool[E.TheProduct.ListOfPathsOfThePhotos.Count]);
}
}
}
else if ((SearchTerm.Contains(" Or ") ^ SearchTerm.Contains(" or ")) && StringArray1.Length >= 2)
{
foreach (Entry E in AllEntries)
{
foreach (string part in StringArray1)
{
if (E.TheProduct.BrandName.Contains(part) ||
E.TheProduct.ModelName.Contains(part))
{
List<Bitmap> NewBitmaps = new List<Bitmap>();
foreach (string FullFileName in E.TheProduct.ListOfPathsOfThePhotos)
{
if (System.IO.File.Exists(FullFileName))
{
NewBitmaps.Add(new Bitmap(FullFileName));
}
}
// NEUE Instanzen erzeugen, sonst wird auf denselben Speicherbereich gezeigt.
// Create NEW instances, otherwise the same memory area is pointed to.
search_results.Add(new Entry(
E.Employee_who_created_Entry,
E.CreationTime,
E.Changed_by_Employee,
E.LastChangeTime,
new Product(
E.TheProduct.BrandName,
E.TheProduct.ModelName,
E.TheProduct.Size,
E.TheProduct.DescriptionOrNotes,
NewBitmaps,
E.TheProduct.ListOfPathsOfThePhotos,
E.TheProduct.PurchasePrice,
E.TheProduct.SellingPrice)
)
);
search_results.Last().TheProduct.are_images_loaded = (new bool[E.TheProduct.ListOfPathsOfThePhotos.Count]);
}
}
}
}
else if (StringArray2.Length >= 2) // A, but NOT B
{
foreach (Entry E in AllEntries)
{
if ((E.TheProduct.BrandName.Contains(StringArray2[0]) ||
E.TheProduct.ModelName.Contains(StringArray2[0])) &&
!E.TheProduct.BrandName.Contains(StringArray2[1]) &&
(!E.TheProduct.ModelName.Contains(StringArray2[1])))
{
List<Bitmap> NewBitmaps = new List<Bitmap>();
foreach (string FullFileName in E.TheProduct.ListOfPathsOfThePhotos)
{
if (System.IO.File.Exists(FullFileName))
{
NewBitmaps.Add(new Bitmap(FullFileName));
}
}
// NEUE Instanzen erzeugen, sonst wird auf denselben Speicherbereich gezeigt.
// Create NEW instances, otherwise the same memory area is pointed to.
search_results.Add(new Entry(
E.Employee_who_created_Entry,
E.CreationTime,
E.Changed_by_Employee,
E.LastChangeTime,
new Product(
E.TheProduct.BrandName,
E.TheProduct.ModelName,
E.TheProduct.Size,
E.TheProduct.DescriptionOrNotes,
NewBitmaps,
E.TheProduct.ListOfPathsOfThePhotos,
E.TheProduct.PurchasePrice,
E.TheProduct.SellingPrice)
)
);
search_results.Last().TheProduct.are_images_loaded = (new bool[E.TheProduct.ListOfPathsOfThePhotos.Count]);
}
}
}
else
{
foreach (Entry E in AllEntries)
{
if (E.TheProduct.BrandName.Contains(SearchTerm) ||
E.TheProduct.ModelName.Contains(SearchTerm))
{
List<Bitmap> NewBitmaps = new List<Bitmap>();
foreach (string FullFileName in E.TheProduct.ListOfPathsOfThePhotos)
{
if (System.IO.File.Exists(FullFileName))
{
NewBitmaps.Add(new Bitmap(FullFileName));
}
}
// NEUE Instanzen erzeugen, sonst wird auf denselben Speicherbereich gezeigt.
// Create NEW instances, otherwise the same memory area is pointed to.
search_results.Add(new Entry(
E.Employee_who_created_Entry,
E.CreationTime,
E.Changed_by_Employee,
E.LastChangeTime,
new Product(
E.TheProduct.BrandName,
E.TheProduct.ModelName,
E.TheProduct.Size,
E.TheProduct.DescriptionOrNotes,
NewBitmaps,
E.TheProduct.ListOfPathsOfThePhotos,
E.TheProduct.PurchasePrice,
E.TheProduct.SellingPrice)
)
);
search_results.Last().TheProduct.are_images_loaded = (new bool[E.TheProduct.ListOfPathsOfThePhotos.Count]);
}
}
}
// Einschub: Falls vor dem Bedienen der Suche verworfen wurden, müssen sie nun doch geladen werden. Das ist oben passiert. Hier wird das Boolean-Array befüllt.
//If images were disposed before operating the search, they must now be loaded after all. This is what happened above. Here, the Boolean array is filled.
foreach (Entry search_result in search_results)
{
for (int i = 0; i < search_result.TheProduct.are_images_loaded.Length; i++)
{
if (!search_result.TheProduct.are_images_loaded[i])
{
search_result.TheProduct.are_images_loaded[i] = true;
}
}
}
switch (Sort_Criterion)
{
case Esort_criterion.alphabetical:
{
search_results.Sort((x, y) =>
{
int r = StringComparer.Ordinal.Compare(x.TheProduct.BrandName, y.TheProduct.BrandName);
return r;
});
if(CheckBox_reverse.Checked)
{
search_results.Reverse();
}
break;
}
case Esort_criterion.by_Creation_Time:
{
search_results.Sort((x, y) => x.CreationTime.CompareTo(y.CreationTime));
if (CheckBox_reverse.Checked)
{
search_results.Reverse();
}
break;
}
default:
{
break;
}
}
return search_results;
}
So that you can reproduce this, I’m giving you the code of the classes here.
Entries.cs
using System;
namespace IA
{
public sealed class Entry
{
public int Employee_who_created_Entry;
public DateTime CreationTime;
/// <summary>
/// in years
/// </summary>
public double Age;
public int Changed_by_Employee;
public DateTime LastChangeTime;
public Product TheProduct;
public Entry(int Employee_who_created_Entry,
DateTime CreationTime,
int Changed_by_Employee,
DateTime LastChangeTime,
Product TheProduct)
{
this.Employee_who_created_Entry = Employee_who_created_Entry;
this.CreationTime = CreationTime;
this.Age = (DateTime.Now - this.CreationTime).TotalDays / 365.0;
this.Changed_by_Employee = Changed_by_Employee;
this.LastChangeTime = LastChangeTime;
this.TheProduct = TheProduct;
}
}
}
Product.cs
using System;
using System.Collections.Generic;
namespace IA
{
public sealed class Product
{
public string BrandName;
public string ModelName;
/// <summary>
/// in millimetres
/// </summary>
public struct SSize
{
public UInt32 Length;
public UInt32 Width;
public UInt32 Height;
}
public SSize Size;
public string DescriptionOrNotes;
public List<System.Drawing.Bitmap> ListOfPhotos;
public List<string> ListOfPathsOfThePhotos;
public bool[] are_images_loaded;
public decimal PurchasePrice;
public decimal SellingPrice;
public Product(string BrandName,
string ModelName,
SSize Size,
string DescriptionOrNotes,
List<System.Drawing.Bitmap> ListOfPhotos,
List<string> ListOfPathsOfThePhotos,
decimal PurchasePrice,
decimal SellingPrice)
{
this.BrandName = BrandName;
this.ModelName = ModelName;
this.Size = Size;
this.DescriptionOrNotes = DescriptionOrNotes;
this.ListOfPhotos = ListOfPhotos;
this.ListOfPathsOfThePhotos = ListOfPathsOfThePhotos;
this.PurchasePrice = PurchasePrice;
this.SellingPrice = SellingPrice;
}
}
}
Solution
Quick remarks:
-
Please follow the guidelines. No underscores in names of classes etc., proper casing (pascal/camel),…
-
Don’t put all your code in a single file. Most of your
seekmethod should be in a separate class that returns a result of sort. Separate your UI from your back-end logic. -
You have a
seekAsyncmethod that isn’tasync, and aseekmethod that is. -
Don’t pointlessly abbreviate.
Eis totally unclear. -
Use descriptive name.
StringArray1andStringArray2tell me nothing.StringArray2[0]tells me even less. -
ListOfPathsOfThePhotosis a bad name in several ways. Why not simply call itPhotoPaths?TheProductis also a bad property name: just call itProduct. -
Your code contains repeated blocks. Each time you call
new Entry(I see the same parameters. Have a constructor that accepts anEntryplus any additional data (e.g.NewBitmaps) and do all the necessary actions in that constructor. Same withforeach (string FullFileName in E.TheProduct.ListOfPathsOfThePhotos). -
search_results.Last().TheProduct.are_images_loaded = (new bool[E.TheProduct.ListOfPathsOfThePhotos.Count]);seems like a hacky solution. Why not return a class with one property that is the list of results, and another that is this Boolean? -
Why don’t you use LINQ instead of things like this:
search_results.Sort((x, y) => x.CreationTime.CompareTo(y.CreationTime));? Arguably, even much of your search logic could be replaced by much simpler and easier to read LINQ commands that filter throughAllEntries, you can then use the results of those commands to construct the list ofsearch_results. -
So much of your code looks “ancient”. Did you base this on very old example code?