Code Review Asked by cliesens on December 7, 2020
I’ve just started a course that will require knowledge and experience with Python for future projects, which I don’t have, so I thought I’d give it a go to familiarize myself with it and get some feedback. I got a decent summary of the language’s main features from a 2 hour long overview video (https://www.youtube.com/watch?v=H1elmMBnykA), tried a few small things on my own, then decided to move on to something a bit more interesting.
As the title indicates, the code consists of two classes: Complex, which represents complex numbers, and ComplexTest which is a sequence of unit tests for the Complex class. I am aware that Python supports complex numbers natively. I should also mention that all the unit tests from ComplexTest run properly and pass.
I’m interested in comment about literally any parts of my code, since this is my first time writing Python code. Any and all feedback is welcome!
Finally, one point which irked me a bit was the apparent clash between Python 2 and Python 3, which often made me unsure whether the way I was implementing things was "correct" or not from the Python 3 perspective (which is the one I’m targeting).
I also really miss my semicolons and curly brackets 🙁
ccomplex.py
from numbers import Number
import math
class Complex:
def __init__(self, re=0, im=0):
self._re = re
self._im = im
def __eq__(self, other):
if isinstance(other, Complex):
return self.re == other.re and self.im == other.im
else:
raise TypeError("The argument should be an instance of Complex")
def __neg__(self):
return Complex(-self.re, -self.im)
def __add__(self, other):
if isinstance(other, Complex):
return Complex(self.re + other.re, self.im + other.im)
else:
raise TypeError("The argument should be an instance of Complex")
def __sub__(self, other):
if isinstance(other, Complex):
return self + (-other)
else:
raise TypeError("The argument should be an instance of Complex")
def __mul__(self, other):
if isinstance(other, Complex):
a = self.re * other.re - self.im * other.im
b = self.re * other.im + self.im * other.re
return Complex(a, b)
elif isinstance(other, Number):
return Complex(self.re * other, self.im * other)
else:
raise TypeError(
"The argument should be an instance of Complex or Number")
def __rmul__(self, other):
return self * other
def __truediv__(self, other):
if isinstance(other, Complex):
if self.re == 0 and self.im == 0:
return Complex(0, 0)
if other.re == 0 and other.im == 0:
raise ValueError("The argument should be different from zero")
return (self * other.conj()) / other.mod_squared()
elif isinstance(other, Number):
return Complex(self.re / other, self.im / other)
else:
raise TypeError(
"The argument should be an instance of Complex or Number")
def __rtruediv__(self, other):
if isinstance(other, Complex):
if other.re == 0 and other.im == 0:
return Complex(0, 0)
if self.re == 0 and self.im == 0:
raise ValueError("The argument should be different from zero")
return (other * self.conj()) / self.mod_squared()
elif isinstance(other, Number):
return Complex(other, 0) / self
else:
raise TypeError(
"The argument should be an instance of Complex or Number")
def conj(self):
return Complex(self.re, -self.im)
def mod_squared(self):
return self.re * self.re + self.im * self.im
def mod(self):
return math.sqrt(self.mod_squared())
def arg(self):
return math.atan2(self.im, self.re)
@property
def re(self):
return self._re
@re.setter
def re(self, value):
self._re = value
@property
def im(self):
return self._im
@im.setter
def im(self, value):
self._im = value
def __str__(self):
op = "+" if self.im >= 0 else "-"
return "{} {} {}i".format(self.re, op, abs(self.im))
complexTest.py
from ccomplex import Complex
import math
import unittest
class ComplexTest(unittest.TestCase):
def test_equality(self):
self.assertTrue(Complex(2, 2) == Complex(2, 2))
def test_inequality(self):
self.assertFalse(Complex(1, 1) == Complex(2, 2))
def test_equality_raises_type_exception(self):
with self.assertRaises(TypeError):
z = Complex(2, 2) == "Not A Complex"
def test_negation(self):
self.assertEqual(-Complex(4, 4), Complex(-4, -4))
def test_sum(self):
z = Complex(2, 2)
self.assertEqual(z + z, Complex(4, 4))
def test_difference(self):
z = Complex(4, 4)
self.assertEqual(z - Complex(2, 2), Complex(2, 2))
def test_complex_product(self):
z1 = Complex(4, 4)
z2 = Complex(2, 2)
self.assertEqual(z1 * z2, Complex(0, 16))
def test_product_raises_type_exception(self):
with self.assertRaises(TypeError):
z = Complex(2, 2) * "Not A Complex"
def test_left_real_product(self):
z = Complex(2, 2)
self.assertEqual(z * 2, Complex(4, 4))
def test_right_real_product(self):
z = Complex(2, 2)
self.assertEqual(2 * z, Complex(4, 4))
def test_complex_division(self):
z1 = Complex(4, 4)
z2 = Complex(2, 2)
self.assertEqual(z1 / z2, Complex(2, 0))
def test_division_raises_type_exception(self):
with self.assertRaises(TypeError):
z = Complex(2, 2) / "Not A Complex"
def test_complex_division_raises_zero_division_exception(self):
with self.assertRaises(ValueError):
z = Complex(2, 2) / Complex(0, 0)
def test_real_division_raises_zero_division_exception(self):
with self.assertRaises(ZeroDivisionError):
z = Complex(2, 2) / 0
def test_left_real_division(self):
z = Complex(4, 4)
self.assertEqual(z / 2, Complex(2, 2))
def test_right_real_division(self):
z = Complex(2, 2)
self.assertEqual(2 / z, Complex(0.5, -0.5))
def test_conjugate(self):
z = Complex(2, 2)
self.assertEqual(z.conj(), Complex(2, -2))
def test_mod_squared(self):
z = Complex(2, 2)
self.assertAlmostEqual(z.mod_squared(), 8, delta=10e-16)
def test_mod(self):
z = Complex(2, 2)
self.assertAlmostEqual(z.mod(), 2 * math.sqrt(2), delta=10e-16)
def test_arg(self):
z = Complex(2, 2)
self.assertAlmostEqual(z.arg(), math.pi / 4, delta=10e-16)
if __name__ == '__main__':
unittest.main(verbosity=2)
Looks pretty good.
I see you implemented modulus in mod. It's also called absolute value, and that's the name Python uses. If you implement __abs__, then Python's abs function can use it. Then abs(Complex(3, 4)) would give you 5.0. Just like Python's own abs(3 + 4j) does.
Another useful one is __bool__, which lets you declare zero as false, as is standard in Python. Currently you fail this (i.e., it does get printed):
if Complex(0, 0):
print('this should not get printed')
You could then also use that twice inside your __truediv__ method. Like if not self:.
The (in)equality test could be extended. For example, I'd expect Complex(3) == 3 to give me True, not crash. And then your tests inside __truediv__ could alternatively be like if self == 0:.
You can have a look at what Python's own complex numbers have:
>>> for name in dir(1j):
print(name)
__abs__
__add__
__bool__
__class__
__delattr__
__dir__
__divmod__
__doc__
__eq__
__float__
...
Correct answer by superb rain on December 7, 2020
The following shows what most users would consider unexpected behaviour:
from ccomplex import Complex
a = Complex(5, 4) + Complex(3)
b = a
a.re = -a.re
print(b) # "-8 + 4i"
Values are usually considered to be immutable. Since Python uses objects to represent values, and objects have identity which can be shared, the best practice is to use immutable objects when creating what are normally considered values. This looks like it is modifying a string:
a = "Hello"
a += " world"
But since str does not implement the __iadd__ operator, what Python actually does is interpret the statement as a = a + " world", which evaluates the expression a + " world", and assigns the result (a new object) to a. The identity of a changes as the a += ... statement is executed, since a different object is stored in that variable.
>>> a = "hello"
>>> id(a)
1966355478512
>>> a += " world"
>>> id(a)
1966350779120
>>>
Removing the @re.setter and @im.setter methods would change your Complex class to be publicly immutable. While that is a good start, nothing prevents someone from manipulating the internals directly, like a._re = 7.
The easiest way to make this class truly immutable is to inherit from an immutable base. Assuming you're using at least Python 3.7:
from typing import NamedTuple
class Complex(NamedTuple):
re: float
im: float = 0
def __eq__(self, other):
if isinstance(other, Complex):
return self.re == other.re and self.im == other.im
else:
return NotImplemented
# ... etc ...
The NamedTuple base class automatically creates the constructor for you, so Complex(2, 3) produces your 2 + 3i complex value. If no value is provided for im, such as Complex(2), the default of 0 is used for im.
If you wanted to change the re or im value, you must create a new object.
a = Complex(-8, a.im)
or, using NamedTuple._replace:
a = a._replace(re=-8)
The astute reader will notice return NotImplemented in the above. This is a magic singleton, which is a signal to Python to try alternatives. For instance, a == b could fallback on not a.__neq__(b), b.__eq__(a), or even not b.__ne__(a).
Consider: you may not know about a Matrix class, but it might know about your Complex class. If someone does cmplx * matrix, if your __mul__ function raises TypeError, it's game over. If instead NotImplemented is returned, then Matrix.__rmul__ can be tried, which might work.
See NotImplemented and Implementing the arithmetic operations
When evaluating a / b, first is tried a.__truediv__(b). If that fails (was not defined or returns NotImplemented), b.__rtruediv__(a) may be tried.
class Complex:
...
def __rtruediv__(self, other):
if isinstance(other, Complex):
...
...
Why would isinstance(other, Complex) ever be true? It would mean both that self is a Complex (since we're in Complex.__rtruediv__), and other is a Complex (since isinstance says so in this scenario). But if that is the case, we're doing Complex() / Complex(), so then __truediv__ should have been used, and __rtruediv__ wouldn't even need to be considered.
Why does Complex(2, 2) / 0 raise a ZeroDivisionError where as Complex(2, 2) / Complex(0, 0) raises a ValueError? Shouldn't it be raising a ZeroDivisionError?
Your test name test_complex_division_raises_zero_division_exception doesn't match the with self.assertRaises(ValueError) condition, which suggests you knew what it should have raised, and discovered the error, but changed the test to match the condition that was raised, instead of raising the correct exception.
Answered by AJNeufeld on December 7, 2020
Get help from others!
Recent Answers
Recent Questions
© 2024 TransWikia.com. All rights reserved. Sites we Love: PCI Database, UKBizDB, Menu Kuliner, Sharing RPP