[PATCH] Add "by" options to increment_counter and decrement_counter to make them consistent with increment! and decrement!

+1

+1

+1

making the 'by' value absolute for ensuring the increment and decrement

I think its implementation should be more specific in terms of increment and decrement counters and since we are receiving the value from the application code into rails so we need to make is sure that its an integer.

Added the patch with these modifications.

Hi Sur,

I have to disagree with your modifications. They are at odds with the parallel implementation of increment! and decrement! in AR (take a look at them and you'll see that the same implementation there is the same as mine--no absolute and no to_i.) However, that alone is not proof that my version is better, so let's take a look at each modification in turn.

Firstly, I think using abs would lead to unexpected behavior and is also just conceptually wrong. We shouldn't presume to guess the intentions of the person calling the method. Using abs here would be like if Ruby's subtraction method were to invoke .abs on any number argument passed to it. In that case, 1 - -1 would return 0, rather than 2 (which it should.)

While it may seem counter-intuitive, there are definitely circumstances in which one would want to subtract by a negative number (or add by one). Equally, there are circumstances where one may wish to decrement by a negative number. Consider, (the admittedly contrived) example of a web form called "Add inventory," which shows a list of products and allows one to adjust their amount_in_stock. One might wish to put a positive number in columns where there are items whose inventory we wish to increment and then put a negative number down for those items where we want to decrement the inventory. In the controller handling the form, we'd might call the increment_counter method and therefore get unexpected results. We just have to trust that people decrementing by a negative number are doing so because they have a reason to do so.

On the issue of whether an integer is passed, I think that transparently calling .to_i on it is dangerous. If you pass 3.1 to an increment method, this is clearly an exceptional circumstance (proceeding from your assumptions) and in that case an exception should be raised (if anything) rather than just silently converting it to an integer, which could lead to unexpected results. So if you had to do anything at all, I'd rather raise an exception. That being said, the increment! method elsewhere in AR doesn't raise an exception or do any sort of .to_i conversion. So my first inclination would be to just mimic that behavior and leave it as it is. (For what it's worth, although it slightly defies the definition of the word "increment" I believe that calling increment_counter with a float will actually still work and adjust the value in the db by that non-integer. If an exception needs to be thrown at the database level because of the column type, that will happen irrespective of our code.)

So my vote is actually to leave the first patch as it was. What do you think of the arguments I make in that favor? :)

I am not completely agreed with the explanation but, the option of incorporating the enhancement of additional 'by' in the methods sounds good.

But, if we consider the other options; I suppose the main idea behind providing these kinda methods is only to have the quick change in the record without being worried about passing the parameter. If we want it to increment/decrement according to the value we are providing then its the call for using update_attribute/s probably because those methods are already there to accept two parameters of the attribute_name and the value.

thoughts?

Hi Sur,

Perhaps I should give the background on why I want to add this functionality. The reason what my patch offers is different than update_attribute (and the reason why I believe the counter methods exist in AR in the first place) is that in cases where you have counters, you want every change to be atomic. And single db queries are atomic, which is what the counters methods in AR use.

If you for example, do this:

d=Deliverable.find(@cart_item.id)
Deliverable.update_attribute(:amount_in_stock,d.amount_in_stock - @cart_item.quantity)

Then you've created the possibility of a "race condition" if other processes are doing the same thing, which would cause the objects in memory to be at odds with the objects in the db, and your amount_in_stock numbers would get out of sync.

One way of fixing this is by using pessimistic locking (:lock=>true), but I think that is cumbersome and ugly for a task like this.

The counters methods take advantage of the inherent atomic properties of a single DB query by referencing the existing amount_in_stock in the query itself and adjusting it relative to that number in the db rather than that number as it is reflected by the in-memory objects.

So I would disagree that the main value of the counters methods is to effect a quick change in the record. They provide a single atomic update to the record values (rather than your having to write out the SQL for this by hand,) which is what truly makes them useful.

Does this all make more sense in the light of that purpose?