We’re rewarding the question askers & reputations are being recalculated! Read more.

Hot answers tagged

31

This might be against the spirit of Code Review, but you don't need to write your own single instance manager for WPF! Microsoft has already written code to accomplish this, but it has been poorly advertised. Microsoft's single instance manager is extremely comprehensive, and I have yet to find any issues with it. (And if you don't want to use it, it can at ...


26

Your code unnecessarily sorts the whole collection on each insert (which is likely going to be O(n log n)). It also produces two events for each insert (one for Add and one for Move), which also isn't necessary. What I would do: Find the index where the inserted item belongs. Insert the item at that index. A simple implementation for that could look like ...


20

Unlearn WinForms. Your view has way too many concerns. As @TeaDrivenDev mentioned, XAML is pretty verbose... and WPF bindings will turn your view model into a very combersome mess of getters and notifying setters... if you think in WinForms. Dragging-and-dropping 100-some controls onto a UserForm creates ugly bloated code-behind in WinForms; there's no ...


18

Well, first questions is: why don't you use MVVM? Contents of your App class look like something that should be implemented on model/view model level. This looks like a lot of copy-pasting private void Blue_MouseDown(object sender, MouseButtonEventArgs e) { OnSimonButtonClicked(SimonButton.Blue); e.Handled = true; } private void Yellow_MouseDown(...


17

It's better to use nested grids that are easy to read, than complex spanning. I try to avoid spanning in most scenarios, unless it's just 1 simple span. But when in doubt, nest the grids, because that way future layout changes won't break everything where as spanning is directly tied to the number of columns you have. A great example of this is headers ...


16

All in all, your code is quite well-structured. Here are a couple of suggestions for improvement: Consider moving your styles to a resource dictionary: <Window.Resources> <ResourceDictionary Source="Resources.xaml" /> </Window.Resources> This will de-clutter your file and allow you to re-use the styles in other windows. Collapse ...


15

Magic numbers, you have at least one of them in class App: private async Task PlaySequence() you should extract this to either a constant or better to a property, so that the speed, aka less sleep, can be adjusted. I don't see the point of the _seed variable. Yeah, I know why you use it, but it just forces you, each time the GenerateSequence() method is ...


13

I don't like a class that's just called Helpers - that's generally a code smell, something that ends up a big dumping ground for anything that doesn't quite fit anywhere else. Be more specific when naming things, perhaps VisualHierarchyHelper would be a better name? I'm using a very similar method - the main difference is essentially the number ot return ...


12

Please, pretty please, use braces. They are totally lacking in your code. if (!itemresult) if (ForeachChildIn(item, item.getChilds().ToList())) result = true; else if ((Visibility)item.GetValue(FrameworkElement.VisibilityProperty) == Visibility.Visible) if (item is Control) ((Control)item).Background = Brushes.LimeGreen; ...


12

Scratching the surface... Naming I have to second @BCdotNET's comment, the "cl" prefixes are Hungarian, confusing and against C# naming standards. Types should be named in PascalCase; the lowercase prefix makes for very confusing code - even the syntax highlighter is confused! private clDelegateCommand _EditCommand; private clDelegateCommand _SaveCommand;...


11

WPF readability To keep your xaml looking clean, I reccommend using self closing tags <RowDefinition /> vs <RowDefinition></RowDefinition> Explicitly set properties instead of using the > content area < of a tag. Doing such more explicitly shows what the content is used for. <Label Grid.Row="0" Grid.Column="0" Content="First ...


11

After reading some of the comments and looking at a few things I think the underlying question is still unanswered. I'll break this answer up into 2 sections. One unit testing, and two smaller code. Unit testing is TONS easier to do with switching to MVVM. The view model already needs to be public, as well as all the properties. As you progress in your ...


11

Take a look at arrays. It will allow you to quickly add and remove future options without needing to change the logic of the code. It will also allow you to possibly add more complex logic in a single place, instead of having to add code to every if/switch statements. string[] options = { "Microsoft Hazel Desktop", "Microsoft Heera ...


11

I'm glad to see someone did a WPF calculator, and I hope that I can add to what was already stated. Before I get started: "yes this is a MVVM rant". Now that I got that out of the way, I can understand though why a person would do a code behind approach. One is that it is what a ton of people are used too (I was one of them coming from Java, then going to ...


10

View The XAML is rather... simplistic: Each label and textbox (and the Save button) stretches all the way across the window. Instead of just dumping controls into a StackPanel, you could consider leveraging automagic layout with a WrapPanel, and making each label+textbox duet logically regrouped. I like that you have no WinForms-like absolute positioning,...


10

One way to remove that ugly block of ifs is to use a string[]. You'd define it like this: string[] voices = { "Microsoft Hazel Desktop", "Microsoft Heera Desktop", // etc. }; Then, in place of your ifs, write this: synth.SelectVoice(voices[comboBox.SelectedIndex]); Much cleaner. Also, C# convention states that method names are PascalCase, ...


10

I think ObjectBase name is too generic. Class name should reflect its purpose. ViewModelBase is an example of better naming. Same goes for FirstViewModel and SecondViewModel. Using as operator after using is should generally be avoided. You should either use is and then use strong cast, or use as and then a null check. The second option is usually better ...


10

Initial observation is that you should avoid async void like private async void start() { //... } except for event handlers. Reference Async/Await - Best Practices in Asynchronous Programming Luckily there is one for the start button. So start by refactoring start to be proper async private async Task start() { //... } and awaiting it in the ...


10

There are a couple of things which could be more consistent. public static readonly DependencyProperty StartAngleProperty = DependencyProperty.Register("StartAngle", typeof(double), typeof(Arc), new FrameworkPropertyMetadata(0.0, FrameworkPropertyMetadataOptions.AffectsRender)); public static readonly DependencyProperty ...


9

First the simple stuff: Standard naming convention for methods in C# is PascalCase while you run a mix of camelCase and PascalCase. Some of the methods have unclear names and/or unclear parameter names. For example this: void Button(int p1, int p2, char[,] board) What is p1 or p1 and what should the method do? The names should be descriptive enough to ...


9

Sorry to disappoint, but you did everything right! This is a quite good example of using RxUI, Akavache, and Refit together. The only thing I would change, is to not immediately call LoadItems.ExecuteAsyncTask in the ViewModel constructor. Invoking this in the VM constructor means that your VM class becomes more difficult to test, because you always have ...


9

Your code looks pretty got but there are some things that you can simplify, probably the switch cases are the most unwanted part of your code. Verbatim strings "Path must start with \"/\"." Can become @"Path must start with ""/""." get only properties private string _path; public string RelativePath => _path; Can become public string RelativePath ...


8

The duplication problem can be solved using a custom UserControl, to encapsulate the controls used to edit a Student, as well as the validation. To define the control: right click you project in the Solution Explorer select Add -> UserControl... enter a name for the control (ex: StudentControl) press the Add button After adding the user control, you can ...


8

As I mentioned in the comments, the code you've posted could very well be doing anything that could potentially throw any exception. MainWindow window = new MainWindow(); By putting this constructor call outside the try block, any exception thrown in the MainWindow constructor will be unhandled and the program will terminate in a not-so-gracious way. I ...


8

Duplication if (totalled == true) { Display.Content = ""; totalled = false; } Display.Content += "1"; storageVariable += "1"; if (totalled == true) { Display.Content = ""; totalled = false; } Display.Content += "2"; storageVariable += "2"; All the way up to 0. (1-9 + 0) If you can somehow figure out which button caused the click event, you ...


8

I would like to add a few thoughts on WPF part of your code: If you need your buttons to occupy half the screen, you should simply replace StackPanel with Grid: <Grid> <Grid.ColumnDefinitions> <ColumnDefinition/> <ColumnDefinition/> <Grid.ColumnDefinitions> </Grid> This is not the case where you ...


8

Alright, let's go through the code top-to-bottom. using Brushes = System.Windows.Media.Brushes; using SystemFonts = System.Windows.SystemFonts; Any particular reason you're using this alias? Most people I see that use aliases (including me) have these as part of an automatic re-formatting of the document. private Microsoft.Win32.OpenFileDialog dlg = new ...


8

Here're things that I think should be improved: private double a = 0; // the first number private double b = 0; // second number Encapsulate those in a class/struct like UserInput public void HandleDigit(int i) i is not a good parameter name, it should be called digit if this is what your method handles. lastOper I think names shouldn't usually ...


8

XAML: First I would recommend that you use the Grid.RowDefinitions and Grid.ColumnDefinitions more / better. Then all the mess with VerticalAlignment, Height etc. can be deleted. But maybe you want a specified Height then you still can do it: <RowDefinition Height="50"/> Then the XAML code looks much cleaner and e.g. like this (but it is a bit ...


8

This could be a better/cooler way: return (parameter as string)?.Split(',').Contains(value as string) ?? (object)false; Your example is so short that I don't know what else I could say. You are looking for a short solution, so here is one.


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