Hot answers tagged

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 ...


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;...


12

You can build the LINQ query in several steps by appending new where clauses IEnumerable<receipt> query = ReceiptList; if (customer != null) { query = query.Where(x => x.CustomerId == customer.ID); } if (number != null) { query = query.Where(x => x.Number.Contains(number)); } if (date != null) { query = query.Where(x => x.Date == ...


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 ...


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

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 ...


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 ...


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

If you start thinking with code, you will almost always end up doing bad things. Why? Because it is not natural. OK, no more preaching and let's get to work. Review of Your Code I apologize for being blunt. You are not using C# and .NET coding conventions. You are using Hungarian Notation: m_userInfoViewModel. You are naming your variables with type info ...


7

T ToEntity(); To me, ToEntity() implies some sort of conversion action. A better option might be T GetEntity() or even a property called Entity. protected readonly T EntityType; EntityType is a bad name for this field, because it does not contain a type, it contains the entity. Because of that, something like Entity might be better. Also, you might want ...


7

I've used the below for a while: public abstract class PropertyChangedBase : INotifyPropertyChanged { public event PropertyChangedEventHandler PropertyChanged; protected void RaisePropertyChanged(string propertyName) { var propertyChanged = this.PropertyChanged; if (propertyChanged != null) { propertyChanged(...


7

Picking up some of what Mat's Mug left clDelegateCommand public event EventHandler CanExecuteChanged { add { CommandManager.RequerySuggested += value; } remove { CommandManager.RequerySuggested -= value; } } I use to call CommandManager.RequerySuggested -= value; before I add it. Doing this won't lead to any problems, if the same value hasn't ...


7

First, I want to point out the behavior of method that is declared as async void. When the code reaches this line: var result = await data; The code following after _service.GetAnimateur is executed. async void is there only for event handlers. So using this signature creates more problems than it solves. Second, I may not see the whole picture, but your ...


7

TestMethod The Result property (and IsEditing property) public TestResult Result { get { return _result; } set { _result = value; OnPropertyChanged();} } seems only to be set by the TestMethod object, so there is no need to have a public setter there. Making the setter either private or protected will increase its encapsulation. The ...


7

General Your model should not return an ObservableCollection because that collection should be used only if its change notification ability is needed (e.g. with data binding). The method GetDocuments should return an Array or an IEnumerable. The method InitializeColumns also seems to be GUI related. Consider moving it to the view model. Background ...


7

An EF DbContext is an IDisposable object that should be as short-lived as possible. There are a number of problems with this: private MoviesContext context = new MoviesContext(); The MoviesSet class owns the MoviesContext instance, and should be responsible for properly disposing it. If the context is going to live as long as the MoviesSet class, then ...


7

I don't feel like you should override + and -. Fine, it makes the syntax cuter, but it makes your code much less readable for someone who isn't you. Remember, you write code so that yourself in 5 years or someone else can read it properly, not so you can feel it better right now. Follow this guideline, and you'll see that your code will be much cleaner. By ...


6

I currently create a new derived collection each time Volatilities changes This was a good idea, but in this case I'd just recreate the brokers list (since it's not a straight volatilities.Select(x => ...), it's a SelectMany. One thing that's important though, is whether you mutate (i.e. add and remove) items from Volatilities, or you only replace the ...


6

DataContext = _ball; RightPad.DataContext = _rightPad; LeftPad.DataContext = _leftPad; Ball.DataContext = _ball; label4.DataContext = _rightPad; I don't think you should be setting DataContext for each control like this. Instead, you should have a view model object that you set as the DataContext of the whole window. That view model would contain all the ...


6

I think that the IsNewWorkOrder property is fine, but there are a couple of other things you should watch out. Why do you have the logic to interact with the database in your ViewModel? I think it will be much cleaner if you extract that to another class and then you inject it as a dependency. You would need something like: interface IDatabase { bool ...


6

Why don't you just use the overloaded constructor of the NetworkCredential class which takes the password as SecureString? In this way you can remove the ConvertToUnsecureString() method at all and you don't have to read the decrypted value of the password. It is always recommended to use System.IO.Path.Combine() instead of adding up the path by using ...


6

Your code looks fine except for two points: a Dispose() method should never throw an exception but yours will likely throw one if it is called twice. This can be avoided by having a private bool disposed variable which is checked by an if condition. an event based method OnXXX is usually either private or protected and used to actually raise the event. ...


6

You should avoid code duplication in constructors. For larger objects this practice can quickly spin out of control. Re-use existing constructors instead: public AsyncCommand(Func<CancellationToken, Task> action) : this(action, () => true) {} or use default parameters: public AsyncCommand(Func<CancellationToken, Task> action, Func<bool&...


6

Design review or code review? Well, in real-world projects we communicate the design in its own way first, not just say “here’s code that implements my design.” I would like to see a paragraph explaining how it works in very simple overview, and a sketch showing the different types and their relationships (UML pseudo-code). Expressions.h You might put ...


6

if (TaskCancellation == true) { break; } CancellationToken.ThrowIfCancellationRequested(); Some comments explaining why two different cancellation methods are necessary would be useful. if (jockey.Name != null) { lock (((...


5

Use IBaseEntity, reflection should never be used unless you really need to. I'm not a fan of returning a not executed IQueryable<> as you do in your Find method, you should consider executing the query and returning the IEnumerable<>, otherwise you expose yourself to potential bugs (If the client of the method messes up with the query) and it is ...


5

Comments I need to comment on ...your comments. Really. NONE of the comments you have here are useful. None of them. Xml comments This is rather unusual: /// <summary>ViewModel for the character view.</summary> Normally XML comments looks like this: /// <summary> /// ViewModel for the character view. /// </summary> That's also ...


5

MVVM And is this actually considered MVVM? I would say parts of your implementations are parts of MVVM. View Is represented by SmallWindow.xaml and is decoupled from the code behind and any business logic. Checked. ViewModel Is represented by ViewModels and provides the binding and presentation logic for the View. Checked. Model Processing is ...


5

Disclaimer: I don't review Xaml, I focus on the C# part only. [SettingsSerializeAs(SettingsSerializeAs.Xml)] public class Game : IEntity { public string Name { get; set; } public string ExecutablePath { get; set; } // e.g. Minecraft.exe public Game() { } // Required to be serialized as a setting. public Game(string name, string ...


5

Looks pretty good. Some readability improvements: Magic character '.' should be moved into a named constant like DefaultDelimiter. Since you need to classify the DeclarationType another way to encapsulate this logic is to create a set of extension methods like this: public static class DeclarationTypeExtensions { public static bool IsComponentType(this ...


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