Hot answers tagged

14

You're throwing System.Exception. Don't do that. If you're going to have to throw an exception for a validation exception, throw a custom ValidationException exception. You haven't shown the code where you catch and handle that exception, but it's going to have to look like this: try { // some code } catch (Exception exception) { // handle the [...


13

Short Answer: Having a call to MessageBox.Show() in any DAL code is A Bad Thing (TM). Longer Answer: In an ideal world, your DAL would not know anything about your presentation layer - not even that it is a Windows/Winforms application. It really should know very little (if anything) about any code that invokes it. I would have the above routine return ...


13

Formatting formatting inside your foreach should be indented This: foreach (Earning e in earningList) { // Sum all earnings... var totalEarnings = earningList[i].AmountForEPF + earningList[i].OverTimeAmount + earningList[i].IncentiveAllowance + earningList[i].SpecialAllowance + earningList[i].OtherAllowance + earningList[i]....


10

It seems to me that you're hard-coding far too much here. IMO, this type of task should be heavily database driven. For example, each item of earning and each deduction should have an amount, and a category. The category should come from a table in the database. So, you'd end up with one table of categories of deductions including the ones you have above (...


10

Another option would be to handle the Validating event: private void textBox_Validating(object sender, CancelEventArgs e) { TextBox currenttb = (TextBox)sender; if(currenttb.Text == "") MessageBox.Show(string.Format("Empty field {0 }",currenttb.Name.Substring(3))); e.Cancel = true; else { e.Cancel = false; } } ...


8

private void ValidateAttendance() { //DataService method returns true if the attendance is valid. var validity = _DataService.CheckValidityOfAnAttendance(_View.EmployeeID, _View.Date, _View.ShiftType); //Set the validity of the attendance in a View property. //So that View can stop execution if validity is false. _View.AttendanceValidity =...


8

(Not a full review, just focusing on one method.) I think private string GetImageKeyForDeclaration(Declaration declaration) contains a lot of repeated code that can be significantly reduced. First of all I'm not a fan of the "use a switch to set a value that you then return"-approach: just do the return directly, especially if there are a lot of cases. I'...


8

class Generator internal Generator(int size) { if (size % 2 != 0) { throw new Exception("Board size must be even!"); } else if (size < 2) { throw new Exception("Board size must be positive!"); } this.size = size; random = new Random(); } The correct exception would be an ...


7

For the simplicity I'm not using a "model class" for this [...] Then it's not Model-View-Presenter you have here. var Presenter = new AttendancePointPresenter(AttendancePointView, ds); I love it. What you've done here, is called Dependency Injection - more specifically, constructor injection. It's a technique that, when applied consistently, greatly ...


7

You don't need these lines of code at all if (sqlConnection.State == System.Data.ConnectionState.Open) sqlConnection.Close(); return false; The point of using the using block is that anything called with that will automatically be closed, so there is no point in closing the connection. leaving that line of code won't hurt anything, but none of this code ...


7

There's usually very little I can say about your code, but maybe if I just start going down line by line I'll find something. I'm hoping someone else who knows more about mvp and dependency-injection come along as well. Simple Form I don't see Types uses very often in vba. Normally when I do, what is really needed is a class. I don't think that's the case ...


6

It looks like you have a pair of lists which you iterate through based on the elements contained in the earningList. This is potentially dangerous without validation. When the earningList is longer than deductionList, you will run into an IndexOutOfRangeException. var earningList = _DataService.GetEarningsList(); //DataService returns a List<Earning> ...


6

You don't want to pull the entire Employee table unless necessary. public IEnumerable<Employee> GetNewEmployees() { string selectStatement = "SELECT Employee.Emp_ID, Employee.Initials + ' ' + Employee.Surname AS Name,..."; using (SqlConnection sqlConnection = new SqlConnection(db.GetConnectionString)) { using (SqlCommand ...


6

Fields I see a property Cells but it is never used either in Grid or any other class (as far as I can tell). Order your class members like this: Constants Fields Properties Methods This makes it easier for people to read the code and have an expectation of where they can find what. Naming index and index2 aren't very descriptive names. Consider naming ...


6

private void cmbSave_Click(object sender, EventArgs e) { if(IsFilled (txtApplicationNumber.Text)) { if(IsFilled (txtEmployeeID.Text)) { if (IsFilled(txtNIC.Text)) Save(sender, e); } } } As @Malachi pointed out, the nesting is easily avoidable here. However it's ...


6

I really like this method of declaring a bitwise enum. Public Enum AnchorEdges LeftAnchor = 2 ^ 0 TopAnchor = 2 ^ 1 RightAnchor = 2 ^ 2 BottomAnchor = 2 ^ 3 AnchorAll = LeftAnchor + TopAnchor + RightAnchor + BottomAnchor End Enum I find it much preferable to the way I usually see it done. Public Enum BitWiseEnum SomeValue = 1 ...


6

public abstract class Presenter<V, VM, M> where V : IView<VM, M> where VM : ViewModel<M> where M : IModel Convention for single-letter generic type parameter is to start with T, followed by U. If you're going to have more than that, In any case, it's better to give them meaningful names, and to start them with a T to stick ...


6

Seemed fine at first, then I took another look... The validation function displays the error message? I'm sorry, but that's indeed a violation of SRP. You'd be better off returning a ValidationResult which you could use. You already HAVE this validation result... so in a way, you've already done what I said. What's wrong is that your function names do not ...


6

Your view is calling methods on your presenter. The view shouldn’t know the presenter exists. Instead of calling the presenter directly, the view should raise events that the presenter reacts to. The difference seems trivial for what you have here, but it can make quite a large difference on a larger more complex system. Oh, and I should mention a few ...


5

This method smells: public Earning AddEarning(int employeeID, int workDays, int dayOffs, int leaveDays, int extraShifts, decimal extraShiftsAmount, decimal basicSalary, decimal budjetoryAllowance, int noPayDays, decimal lessNoPayAmount, decimal amountForEpf, decimal overTimeAmount, decimal broughtForwardAmount, decimal ...


5

There's an itchy spot with extensibility: because an implementing class must implement all members of the IPresenter interface, adding new commands is quickly becoming a painful experience. Say you have a view that has a MoveUpButton and a MoveDownButton. With the way it is right now, you'll have to add 4 methods to the IPresenter interface, and modify ...


5

You shouldn't throw Exception, create a custom Exception as proposed @Mat's Mug and throw that one instead, otherwise you might trap an exception you didn't want to trap (ex : MyDatabaseJustExplodedException)! I'd add that it is never good to have as much lines of comment as you have lines of code! Basically, comments shouldn't explain what you are doing ...


5

I'm not going to make a full review either - I'll leave that to other eyes. One thing I just have to mention is this part of the async void RefreshTreeView method: foreach (var vbProject in projects) { var project = vbProject; await Task.Run(() => { var node = new TreeNode(project.Name + " (parsing...)"); node.ImageKey = "...


5

It's hard to review good code, and this is relatively good code. Things are generally well named and no methods are overly long. You've done well, but there is room for improvement. Let's start at the entry point, and this question. What is the correct way to start the application from Program.cs as what I have doesn't seem correct? Nope. This is ...


5

Your algorithm for getting a collection of random deltas could be improved. It checks for collisions each number it adds, which is O(n) each time, and it keeps creating a new random number until there isn't a collision, which could go on forever. Here is an algorithm that is O(n): static class RandomExtensions { public static void Shuffle<T>(...


5

To begin with... public static class ExceptionBuilder { public static void CheckArgumentRangeInclusive(string varName, int value, int lowerRange, int upperRange) { if (value < lowerRange || value > upperRange) throw new ArgumentOutOfRangeException(varName); } } This is not a builder. It's a validator so I suggest naming it ...


4

Your Question System.Data.DataTable lives under the System.Data namespace; one good way to start mixing concerns in your BLL is to reference System.Data, and then you don't even need a DAL any more and can have your BLL directly access the database. Or worse, have some of your data access code code in the DAL, and then some more data access code in the BLL. ...


4

It's not clear exactly what you're referring to as Model classes, I think you're over-thinking it. I'd go with the else snippet, the POCO (Plain Old CLR Object) that does nothing but exposing auto-properties. The snippet that exposes a public Advance field doesn't look right. Do not expose public fields; a class should expose properties, not fields. Your ...


4

This question is very, very similar to another question you recently asked, so this answer will basically (hopefully!) clarify my other answer. KISS: Keep It Simple, Stupid. These classes represent database records. With Entity Framework you would define the relationships like this (I'd drop the Info suffix): public class Deduction { // primary key: ...


4

I think you have a typo in the last outside if statement, I assume you meant two different variables when you wrote this: if((!string.IsNullOrWhiteSpace(this.View.FromCustomerId) && !string.IsNullOrWhiteSpace(this.View.FromCustomerId) && (isValidFromCustomerId && isValidFromCustomerId)) { if(parsedFromCustomerId > ...


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