11
\$\begingroup\$

I have following code to create a mutable namedtuple. based my understand I can use dataclass to do it. is there a better way to do it or clean up the code?

@dataclass
class Price:
    """
    This describes how to map default price value for product
    """
    profit: float = 0.5
    cost: float = 0.1
    sale: float = 25.0

    def round(self, n: float):
        if n < 1:
            return round(n, 2)
        elif n < 100:
            n = round(n / 1)
        elif n < 1000:
            n = round(n / 5) * 5
        elif n < 10000:
            n = round(n / 50) * 50
        else:
            n = round(n / 500) * 500
        return n

    def update(self, **kwargs):
        rate = kwargs.get('rate', 1)
        for k, v in asdict(self).items():
            if k != 'sale':
                v = self.round(v * rate)
            v = kwargs.get(k) or v
            setattr(self, k, float(v))

#run test
p = Price()
p.update(rate=1) #p = Price(profit=0.5, cost=0.1, sale=25.0)

p = Price()
p.update(**{sale=450}, **dict(rate=0.9073)) #p = Price(profit=0.45, cost=0.1, sale=450.0)

p = Price()
p.update(**{sale=800}, **dict(rate=301.377)) #p = Price(profit=150.0, cost=0.1, sale=800.0)

p = Price()
p.update(rate=301.377) #p = Price(profit=150.0, cost=0.1, sale=7550.0)

p = Price(0.0, 0.5, 50.0)
p.update(rate=1) #p = Price(profit=0.0, cost=0.5, sale=50.0)

p = Price(0.0, 0.5, 50.0)
p.update(**{sale=600}, **dict(rate=0.9073)) #p = Price(profit=0.0, cost=0.5, sale=600.0)

p = Price(0.0, 0.5, 50.0)
p.update(**{sale=1200}, **dict(rate=301.377)) #p = Price(profit=0.0, cost=0.5, sale=1200.0)

p = Price(0.0, 0.5, 50.0)
p.update(rate=301.377) #p = Price(profit=0.0, cost=0.5, sale=15000.0)
\$\endgroup\$
  • \$\begingroup\$ Any particular reason you're not using python's built-in round function or Decimal class? \$\endgroup\$ – scnerd Sep 12 at 17:44
  • 2
    \$\begingroup\$ It's fairly clear what you mean by this question, although there are some terminology issues. Tuples by definition are immutable. \$\endgroup\$ – Reinderien Sep 12 at 22:13
13
\$\begingroup\$

In short:

\$\endgroup\$
  • \$\begingroup\$ what should I use build-in round version ? based on the logic of round I have? \$\endgroup\$ – jacobcan118 Sep 12 at 18:01
  • 1
    \$\begingroup\$ Because the builtin version does all yours does, and is tested against all manner of edge cases that yours is not. And if it ever becomes important, it's also faster. So you can just remove the method. \$\endgroup\$ – Gloweye Sep 12 at 18:09
  • 1
    \$\begingroup\$ so how can i use to round in the same logic? 102 -> round to 100, 103 -> round to 5 , 1023 -> 1000, 1044 -> 1050 \$\endgroup\$ – jacobcan118 Sep 12 at 21:03
  • \$\begingroup\$ @jacobcan118 (Decimal("1206")/10).quantize(Decimal("1"))*10 == Decimal("1210"). Cumbersome, but surprisingly quantize doesn't seem to support rounding integers. I'd love if someone has a simpler solution (not involving round, since it converts to floating point). \$\endgroup\$ – l0b0 Sep 13 at 11:58
4
\$\begingroup\$

The line v = kwargs.get(k) or v means you cannot use the update method to set any of the properties to 0. get can take an additional argument for the default value to return. If you change that line to v = kwargs.get(k, v), then price.update(sale=0) works as expected.

\$\endgroup\$
  • \$\begingroup\$ Good catch. It is my bug \$\endgroup\$ – jacobcan118 Sep 14 at 12:12
1
\$\begingroup\$

I suggest some correction and simplification of your update method.

Result:

def update(self, rate=1, **kwargs):
  self_dict = asdict(self)
  self_dict.update(kwargs)

  for k, v in self_dict.items():
    if k != 'sale':
      v = self.round(v * rate)

    setattr(self, k, float(v))

Explanation:

  1. The rate = kwargs.get('rate', 1) can be replaced to the rate=1 keyword argument. It does the same: if the rate argument was passed - use it, otherwise use default value 1.

  2. The v = kwargs.get(k) or v line doesn't work as you want I suspect. I found two problems:

    • Test your code with these arguments:

       p.update(sale=450, rate=0.9073, cost=1023)
      

      The rate and round function wouldn't affect the cost. In fact, none of passed keyword arguments will be processed.

      Why? Because the v = kwargs.get(k) or v says literally: get keyword argument named k from the kwargs dictionary and assign it to v. If kwargs doesn't have such item use v (previously processed in the for loop). See? If k argument was passed it is used directly - bypassing processing.

    • Also, as was mentioned in other answer, you can't pass 0 as value of keyword argument, because in this case the kwargs.get(k) will return 0, which is interpreted as FALSE and the v will be used.

  3. Both asdict(self) and kwargs are dictionaries, so we can use the dictionary's update method to replace current self values to passed keyword arguments:

    self_dict = asdict(self)
    self_dict.update(kwargs)
    

We can do this at the beginning of function and then use updated, relevant dictionary further.

Also, I think the round function should be more predictable. Now, it returns float in one case, int in others...

\$\endgroup\$

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.