Private Sub BasePriceEmptyWeight_CustomDisplayText(ByVal sender As Object, ByVal e As CustomDisplayTextEventArgs) Handles txtBasePrice.CustomDisplayText, txtEmptyWeight.CustomDisplayText
If e.Value = "0" Then
e.DisplayText = String.Empty
End If
End Sub
After some research, I found that the controls are bound to a properties in VehicleDetail named BasePrice and UnloadedWeight.
Me.txtBasePrice.DataBindings.Add(New System.Windows.Forms.Binding("EditValue", Me.VehicleDetailsBindingSource, "BasePrice", True))
The following is the code on how they are defined:
Public Property BasePrice() As Double
Public Property UnloadedWeight() As Integer
and the respective column in the database is nullable numeric
[BASE_PRICE] [numeric](9, 2) NULL,
[UNLOADED_WEIGHT] [numeric](9, 0) NULL,
the defect says when I do not have a base price or UnloadedWeight and do not need them, I do not want to see their value as 0.
well, that would be difficult, because Double/integer are value types , even you assign Nothing (null) to it, it will have value zero. so we came out with the idea of faking the value in UI to turn zero to String.Empty.
With this solution, the user would not see zero in the UI, but internally, the value of the base price is still zero. In the database, zero still get to be written into database even it is a nullable column. Besides the confusion it would cost in understanding data and understanding the code, ( one would ask the data source is having numeric type, why we change a valid numeric value to a non-valid numeric value? what about “00” or “000”?) I was wondering how many UI places we need to fake? This one defect need to be fixed in how many places ? Do we know where are they? How many rounds we need to go through before we can claim we have indeed fixed this defect.
I thought the proper solution to the defect could be changing the type of BasePrice / property from Double to Nullable Double and change UnloadedWeight from integer to nullable integer. when their type is changed, in the entire system, when their value is nothing, The binding source will automatically put nothing on the control instead of zero. and when the object is persisted to database, Null get to be persisted to the database table.as well. This defect would not be reopened or created because in some other places we missed. Granted, this might take a bit more effort to do, but when it is fixed once, it will be fixed for good.
I thought there are technical and economical motivation for us to fix the root cause of the defect instead of proving quick fix as we did. It might be acceptable if we do that in production support branch, but it would not be a good choice in product development branch. We might have some justifications for doing what we did in this case, but in general, I would say it is not encouraged in software development project. I am here calling upon all of us : Fix it once, fix it well and fix it for good when it comes to defect fixing.
However, I would think this is not completely the responsibility of the development team, the QA team may have its share as well. if the defect is filed as following:
the business requirement of the system indicated that when Base Price is not applicable, the system should not display zero for base price. but in this specific screen, we see zero in such scenario. which need to be fixed.
In this case, the defect described a general business rule and cite a specific case where the rule was not followed. It indicates that this is case where the rule is broken, the system not only need to make sure in this case the rule is not broken, but also need to ensure the rule is not broken in the entire system. so when we fix this defect, we not only make sure the system acts as designed in this case but also need to ensure it acts correctly in all cases.
However, if the defect was written in another way, the interpretation would be different.
In this screen, I see zero for Based Price when Base Price is not applicable, which is wrong, please hide zero in the screen.
First, it did not describe general business rule or business requirement, it rather focus on this specific screen and specific scenario. it seems suggesting everywhere else is good, and just this place we have a small problem. When the defect is written in this way, it would be very attempted for the development team to do what it is done for this case . However, if our development team aim to go above and beyond, we may want to ask back, how about other scenario, what is the general business rule with regarding to this piece of information.
Take another example here:
Let’s say I was given a task to write an function to calculate f(x, y) = x power of y. (for example, when x = 1, y = 1, you get 1, when x = 2, y = 2 you get 4 and so on,,,)
but I have returned the math Learned in the college or high school to my teachers, I did not get what is power means here. but I have to finish the work assigned to me. looking at the example data, I figure it is just about x + y. maybe Power is just a fancy word here. so I did.
Not soon after I claim have completed the function, I got a defect , the tester says when I put 2 and 3 in, I got 5 , which is wrong, the right answer is 9.
I thought , it looks like in all other cases the function is doing what it is supposed to do, but only when x=2 and y = 3, they want to see 9. instead of 5, then I quickly jump into the code and add an if statement in it.
if x = 2 and y = 3 return 9 else return x + y;
Going on in this way, many defects got filed and got quickly fixed. the code becomes a long haul of if statement and at final else we still have return x + y.
Now, you are coming to the picture, you ask me: Peter, we may be able to take out these big if-else and just to return Math.Pow(x,y). That does not sit well with me. First of all, you mean all the hard work I have put in for months can be replaced with one line? secondly I am very nervous about the suggestion, the suggested change impact lots of cases. I might need to do lots of testing and maybe I could create more defects… Do you have some sympathy for me? if so, please comment on this article to show your sympathy, that would offer me great comfort…
speaking of “faking” I came across another case this morning, I am attempted to share it with you.
There is a defect say when a transaction is loaded from database, a flag property IsSelected is not set. To fix the problem the code change add a line of code in the Data Access Layer method which is used to load the Entity from the database.
MyEntity.IsSelected = True.
Quick and sweet, the defect got fixed! now the check box in the UI started to show up… I pat myself on my back, job well done!
Is it true? do we know if the user checked that checkbox before the transaction is saved to Database ? Do we know when the data is saved to the database, is the the property persisted to database or not? do we know which column it is saved to do? do we know in the same Data Access Layer method we touched in the code, does the stored procedure retrieve the data from that column? Is this the problem in a Data Access Layer method when the transaction is saved to Database or is it the problem when the transaction is retrieved form the database? maybe there is no such column in the table to hold this piece information at all?
Are we making the system better than it was before in the way we fix defects? By doing what we are doing, when can we get the system completed and accepted with some kind of reasonable quality standard that any software product should have? I have more questions than answers.
P/S: I wrote this post originally on November, 2013, it is until today I realized that I forgot to publish it... so I review it once more and publish it today.
No comments:
Post a Comment