51

Performance [..] winforms is not powerful so it takes 1-2 seconds to refresh the content [..] It's not WinForms because it's actually very fast and I have never had any issues with it. There is a 99.99% chance that the code is inefficient so let's have a look. Expression.Compile() This is what slows the application down and where the bottle-neck is ...


33

SQL Injection alarm Don't use user passed text in your sql statements if you aren't using parameters. From: https://stackoverflow.com/a/7505842/2655508 Using parameters helps prevent SQL Injection attacks when the database is used in conjunction with a program interface such as a desktop program or web site. In your example, a user can directly ...


25

Is decoupling necessary for very small applications? YES!! However I'd correct your statement here: Is following those patterns just adding too much to a program that doesn't really need it? (YAGNI?) It's not so much about YAGNI, than it is about Cargo Cult Programming. However in practice, any small application has the potential to grow and ...


23

I have a general rule: if I see string concatenation with a loop I assume it's not the best way of doing it. Let's look at this: foreach (Location exit in Exits) { description += $"— {exit.Name}"; if (exit != Exits[Exits.Length - 1]) { description += "\r\n"; } } I see this pattern a lot. What you need is Select with string.Join: ...


21

private static string newline = "\r\n"; This is very wrong. And particularly with the open sourcing of .NET leading to the likelihood of C# being uses on non-Windows operating systems increasing, we shouldn't depend on a platform-specific newline signature. The correct approach looks like this: private static string newline = Environment.NewLine; And at ...


20

None of the Above. You've shown 4 different ways of mixing up presentation and data concerns; the 4 approaches only differ in minute implementation details and all suffer the same flaws - what you have here is a God class that knows everything about everything there is to know. Let's think differently. This answer uses the names I would have used instead ...


18

Yeah, sure, I could just get rid of the if's and else's and replace ((Item)list...) with Item item = list... and wrap it all in a single Try{}Catch{}, but I really do hate using Try{}Catch{} unless there is something I am not expecting. And to be honest, using Try{} Catch{} makes me feel lazy and like I'm not doing it properly. That is true. The best ...


18

Cleartext passwords Not a code-specific issue exactly but it would appear that you are storing the user's passwords in clear text. This means that anyone gaining access to the LOGIN_Tab would be handed a list of valid usernames and passwords for your application. This is especially bad when combined with SQL injection vulnerabilities! Consider instead ...


17

public sealed partial class Console Why is the class partial if there's no designer-generated code associated to it? The partial keyword is provided as a convenience, to help organize code by allowing the definition of a class to span multiple files. If there's only 1 file, the partial keyword is useless at best, and confusing at worst. At first glance, ...


17

First of all - you have awful duplication of payment calculation pay = (TheValue * Matatanda + Bata) * 2; you do this in each row. Move calculation to the bottom of method: if (cboFrom.SelectedIndex == 0 && cboTo.SelectedIndex == 0) else if else if //.. pay = (TheValue * Matatanda + Bata) * 2; Next - nothing is changing when you change cboFrom ...


16

Why can't you do: progressLabel.Text = string.Format("{0}%", progressBar1.Value) As @MNZ pointed out I potentially didn't answer the question exactly how the OP asked. So here's a slightly modified alternative option: progressLabel.Text = FormatProgress(progressBar1.Value, progressLabel.Text) private string FormatProgress(int currentProgress, string ...


16

The things to consider when it comes to async event handlers are: Exceptions thrown for the handler might be rethrown on the UI SynchronizationContext, which usually crashes the application. After you raise the event, the handlers won't be completed yet. The execution of a handler might be interleaved with the execution of the code after the raising and ...


16

In addition to Heslacher's answer I would also look at the using statement. It is very important to keep a handle on resources like Sql Connections to ensure they are closed and cleaned up. Link to the MSDN about SqlConnection with an example with the using statement. A SqlConnection object represents a unique session to a SQL Server data source. ...


15

Also I may want to implement shortcut customization in the future, and there is no easy way to find out what keys are mapped to which method so it will have to be rewritten. Stop right here! Extract a class for each command (e.g. instead of a mark_as_fixed method, you'll have a MarkAsFixedCommand class). If you're using WPF you can implement the ICommand ...


15

First thing I would do is change if (mainString.Length == desiredLength) to if (mainString.Length >= desiredLength), since if it's longer it'll just do excess work with the StringBuilder and you may as well return early on all conditions that would satisfy it. I would also combine this try/catch block: try { //ensures process wont deny access if ...


14

The main change is separation of logic and User Interface. Firstly the calculation methods should ideally not reside in UI methods like event handlers. Secondly the calculations should really be in it's own class, (but this application is just small enough that it is not ENTIRELY necessary). in programming we hate Magic Numbers. If you can, move any ...


14

Collision detection You can calculate the collision much easier with the Rectangle structure. Use the Contains method if you are interested in single points that Determines if the specified point is contained within this Rectangle structure. var pictureRect = new Rectangle( pictureBox.Location.X, pictureBox.Location.Y, pictureBox.Width, ...


13

Here may be an decent way of doing what you want: public enum HemEnum { [Description("none")] HemNone = -1, [Description("sewn")] Hemsew = 0, [Description("welded")] HemWeld = 1, [Description("double folded")] Hemdoublefold = 2 } public static void LoadHemCombo(ComboBox cbo) { cbo.DataSource = Enum.GetValues(typeof(...


13

I'm not very familiar with WinForms, or how apps like this are designed, so this will be a short review. Rather than typing out 80 repeated characters in a string, as you did here: private static string line = "================================================================================"; You can do something like this, assuming you have .NET 4.0: ...


13

You could merge your two foreach loops using SelectMany in LINQ. So instead of : foreach (DbEntityValidationResult result in results) foreach (DbValidationError error in result.ValidationErrors) You'd have foreach (DbValidationError error in results.SelectMany(r => r.ValidationErrors)) Now, that's up to you to find it more readable or not, I ...


13

Two words: loops and arrays. There a bunch of variable names that differ only by a number. Whenever you see code like this, you can probably clean it up by creating a collection of those things and looping over them. Basically, you have a checkbox and text field repeated multiple times, and one field where you display the total of all checked fields. ...


12

Naming HemEnum, or any enum name that ends with the word Enum, is a bad name for an enum. Enum types should not contain the word "enum" in their names. Similarly, HemEnum values should not contain "Hem" in their names either. Your HemEnum should therefore read something like this: public enum HemType { None = -1, Sewn = 0, Weld = 1, ...


12

you should use auto properties where you don't need any validation. So all of your properties should use them like instead of private bool isChecked; public bool IsChecked { get { return isChecked; } set { isChecked = value; } } you would use public bool IsChecked { get; set; } this example of really unnecessary tenary expression IsChecked = ...


12

I might have missed it in one of the other answers, but in this code: using (MemoryStream memoryStream = new MemoryStream(recording)) { if (memoryStream != null) memoryStream will never be null, so the check is redundant.


12

/// <summary> /// Closes the console. Warning: output will be lost! /// </summary> public static void Close() { form.Hide(); Clear(); } You say you are closing the form, but you are really just hiding it and clearing the data. Types that implement Form have a Close() method. Put a single class/enum/interface in each file. This will ...


12

Why so complicated? One possible approach is: var f = new Form(); if (f.Controls.OfType<TextBox>().Any(x => string.IsNullOrWhiteSpace(x.Text))) { /*your stuff*/ } A second approach is: var TextBox1 = new TextBox(); var TextBox2 = new TextBox(); var textboxes = new[] { TextBox1, TextBox2 }; if (textboxes.Any(x => string....


12

Security issues are covered, so just a bit on the code itself. You have three nearly identical pieces of code, inside a switch-statement. That seems to be a code-smell. Apart from that, I'm missing a default case. What happens if a user logs in before you have assigned a role? He gets an empty screen, or an ugly error message? How about creating a(n ...


12

I think you're rather over-complicated this for yourself! A much better solution is to use the pattern matched results (which you are already creating (t)) (this requires C# 7.0 still, I believe). foreach (Control c in form.Controls) { switch (c) { case CheckBox cb: cb.Checked = false; break; case ComboBox cb: ...


11

Here are a few observations: In Form1_KeyDown and Form1_KeyUp, the logic would be cleaner organized into a switch statement: private void Form1_KeyDown(object sender, KeyEventArgs e) { switch (e.KeyData) { case Keys.S: PongGame.p1movesDown = true; return; case Keys.W: PongGame.p1movesUp = true; ...


11

I can't resist mentioning this. These comments hurt me. 'Declaration of Variables Public Seconds As Integer 'Store Seconds as Integer Public Minutes As Integer 'Store Minutes as Integer Public Hours As Integer 'Store Hours as Integer Public CurHour As Integer 'Store the Current Hour as Integer Public CurMinute As Integer 'Store the Current Minute as ...


Only top voted, non community-wiki answers of a minimum length are eligible