Any suggestions to make this better?
import math
# BSD License:
"""Copyright (c) 2012, Solomon Wise
All rights reserved.
Redistribution and use in source and binary forms, with or without
modification, are permitted provided that the following conditions are met:
1. Redistributions of source code must retain the above copyright
notice, this list of conditions and the following disclaimer.
2. Redistributions in binary form must reproduce the above copyright
notice, this list of conditions and the following disclaimer in the
documentation and/or other materials provided with the distribution.
3. All advertising materials mentioning features or use of this software
must display the following acknowledgement:
This product includes software developed by Solomon Wise.
4. Neither the name of CRange nor the
names of its contributors may be used to endorse or promote products
derived from this software without specific prior written permission.
THIS SOFTWARE IS PROVIDED BY SOLOMON WISE "AS IS" AND ANY
EXPRESS OR IMPLIED WARRANTIES, INCLUDING, BUT NOT LIMITED TO, THE IMPLIED
WARRANTIES OF MERCHANTABILITY AND FITNESS FOR A PARTICULAR PURPOSE ARE
DISCLAIMED. IN NO EVENT SHALL SOLOMON WISE BE LIABLE FOR ANY
DIRECT, INDIRECT, INCIDENTAL, SPECIAL, EXEMPLARY, OR CONSEQUENTIAL DAMAGES
(INCLUDING, BUT NOT LIMITED TO, PROCUREMENT OF SUBSTITUTE GOODS OR SERVICES;
LOSS OF USE, DATA, OR PROFITS; OR BUSINESS INTERRUPTION) HOWEVER CAUSED AND
ON ANY THEORY OF LIABILITY, WHETHER IN CONTRACT, STRICT LIABILITY, OR TORT
(INCLUDING NEGLIGENCE OR OTHERWISE) ARISING IN ANY WAY OUT OF THE USE OF THIS
SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF SUCH DAMAGE.
"""
class CRange:
"""CRange is short for Changeable Range. With this type of range object, one
can edit the start, stop, and step for the range, as well as easily
retrieving the values in the range"""
def __init__(self, maximum, start=0, step=1):
"""Sets the maximumimum"""
self.maximum = math.ceil(maximum)
self.start = math.floor(start)
self.step = math.ceil(step)
def __iter__(self):
"""Iterates over the range"""
for x in range(self.start, self.maximum, self.step):
yield x
def __str__(self):
"""Returns a string representation of the CRange"""
return "CRange object with Max: {0}, Min: {1}, and Step: {2}".format(self.maximum, self.start, self.step)
def __int__(self):
"""Returns a integer representation of the maximum"""
return self.maximum
def __len__(self):
"""Returns the number of values in the range"""
x = 0
for i in self:
x += 1
return x
def tuple(self):
"""Returns a tuple representation of the maximum, minimum, and step"""
return (self.maximum, self.start, self.step)
def dict(self):
"""Returns a dictionary representation of the maximum, minimum, and step"""
return {"Max": self.maximum, "Min": self.start, "Step": self.step}
def getmaximum(self):
"""Gets the length of the range"""
return self.maximum
def getstart(self):
"""Gets the starting point of the range"""
return self.start
def getstep(self):
"""Gets the step of the range"""
return self.step
def getall(self):
"""Returns all range attributes"""
print("Start:", self.start)
print("Step:", self.step)
print("Stop:", self.maximum)
return 0
def change_range(self, attr, option, x):
"""Provides options to change attributes of the range"""
if option == "add" and attr == "maximum":
try:
self.maximum += x
except:
print("Error")
elif option == "sub" and attr == "maximum":
try:
self.maximum -=x
except:
print("Error")
elif option == "mult" and attr == "maximum":
try:
if self.maximum <= 0:
raise SyntaxError
self.maximum *= x
except:
print("Error")
elif option == "div" and attr == "maximum":
try:
self.maximum /= x
if self.maximum <= 0:
raise SyntaxError
except:
print("Error")
elif option == "add" and attr == "start":
try:
self.start += x
except:
print("Error")
elif option == "sub" and attr == "start":
try:
self.start -=x
except:
print("Error")
elif option == "mult" and attr == "start":
try:
if self.start <= 0:
raise SyntaxError
self.start *= x
except:
print("Error")
elif option == "div" and attr == "start":
try:
self.start /= x
if self.start <= 0:
raise SyntaxErros
except:
print("Error")
elif option == "add" and attr == "step":
try:
self.step += x
except:
print("Error")
elif option == "sub" and attr == "step":
try:
self.step -= x
except:
print("Error")
elif option == "mult" and attr == "step":
try:
if self.step <= 0:
raise SyntaxError
self.step *= x
except:
print("Error")
elif option == "div" and attr == "step":
try:
self.step /= x
if self.step <= 0:
raise SyntaxError
except:
print("Error")
self.start = math.ceil(self.start)
self.step = math.ceil(self.step)
self.maximum = math.ceil(self.maximum)
def shift(self, x):
"""Shifts both the maximum and the start in one function call"""
try:
if self.maximum + x < 0 or self.start + x < 0:
raise SyntaxError
self.maximum += x
self.start += x
self.maximum, self.start = math.ceil(self.maximum), math.ceil(self.start)
except:
print("Error")
def stretch(self, x):
"""Changes both the maximum and the step in one function call"""
try:
if self.maximum + x < 0 or self.step + x < 0:
raise SyntaxError
self.maximum += x
self.step += x
self.maximum, self.step = math.ceil(self.maximum), math.ceil(self.step)
except:
print("Error")
def docs():
"""Documentation for the module"""
print("This module was written for Python 3.2")
print("Text Editors Used: Emacs, Komodo Edit")
print("OS Used: Mac OS X")
A follow-up to this question can be found here.
-
\$\begingroup\$ Better in what way? Be specific. \$\endgroup\$Jeff Mercado– Jeff Mercado2012年04月28日 19:19:04 +00:00Commented Apr 28, 2012 at 19:19
-
2\$\begingroup\$ @JeffMercado, without any specific requests, the assumption of Code REview is that they want general feedback on the code. \$\endgroup\$Winston Ewert– Winston Ewert2012年04月28日 19:33:13 +00:00Commented Apr 28, 2012 at 19:33
-
\$\begingroup\$ @Winston: I don't know about that, we could give a better review if we were given some direction on where he wants to go. "I want this to be faster," "Is my implementation efficient," "I want to use the X design pattern" is a lot better than "How can I make this better?" \$\endgroup\$Jeff Mercado– Jeff Mercado2012年04月28日 19:35:48 +00:00Commented Apr 28, 2012 at 19:35
-
2\$\begingroup\$ Drop the copyright would you? NASA will not use this code and neither will Goldman Sachs. Python code is not meant to take up pages and pages ... \$\endgroup\$Leonid– Leonid2012年04月28日 20:11:03 +00:00Commented Apr 28, 2012 at 20:11
2 Answers 2
import math
# BSD License:
"""Copyright (c) 2012, Solomon Wise
All rights reserved.
Redistribution and use in source and binary forms, with or without
modification, are permitted provided that the following conditions are met:
1. Redistributions of source code must retain the above copyright
notice, this list of conditions and the following disclaimer.
2. Redistributions in binary form must reproduce the above copyright
notice, this list of conditions and the following disclaimer in the
documentation and/or other materials provided with the distribution.
3. All advertising materials mentioning features or use of this software
must display the following acknowledgement:
This product includes software developed by Solomon Wise.
4. Neither the name of CRange nor the
names of its contributors may be used to endorse or promote products
derived from this software without specific prior written permission.
THIS SOFTWARE IS PROVIDED BY SOLOMON WISE "AS IS" AND ANY
EXPRESS OR IMPLIED WARRANTIES, INCLUDING, BUT NOT LIMITED TO, THE IMPLIED
WARRANTIES OF MERCHANTABILITY AND FITNESS FOR A PARTICULAR PURPOSE ARE
DISCLAIMED. IN NO EVENT SHALL SOLOMON WISE BE LIABLE FOR ANY
DIRECT, INDIRECT, INCIDENTAL, SPECIAL, EXEMPLARY, OR CONSEQUENTIAL DAMAGES
(INCLUDING, BUT NOT LIMITED TO, PROCUREMENT OF SUBSTITUTE GOODS OR SERVICES;
LOSS OF USE, DATA, OR PROFITS; OR BUSINESS INTERRUPTION) HOWEVER CAUSED AND
ON ANY THEORY OF LIABILITY, WHETHER IN CONTRACT, STRICT LIABILITY, OR TORT
(INCLUDING NEGLIGENCE OR OTHERWISE) ARISING IN ANY WAY OUT OF THE USE OF THIS
SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF SUCH DAMAGE.
"""
class CRange:
Is abbreviating really helpful? Maybe the name should be ChangeableRange or MutableRange. CRange is kinda problematic because C gets used to mean class in prefixe a lot
"""CRange is short for Changeable Range. With this type of range object, one
can edit the start, stop, and step for the range, as well as easily
retrieving the values in the range"""
def __init__(self, maximum, start=0, step=1):
"""Sets the maximumimum"""
That's not a very good description
self.maximum = math.ceil(maximum)
self.start = math.floor(start)
self.step = math.ceil(step)
Thats a seriously unexpected transformation. Using this will be confusing
def __iter__(self):
"""Iterates over the range"""
for x in range(self.start, self.maximum, self.step):
yield x
Just return iter(range(self.start, self.maximum, self.step))
, don't needlessly iterate over the range yourself.
def __str__(self):
"""Returns a string representation of the CRange"""
return "CRange object with Max: {0}, Min: {1}, and Step: {2}".format(self.maximum, self.start, self.step)
def __int__(self):
"""Returns a integer representation of the maximum"""
return self.maximum
Does converting a range into an int actually make any sense?
def __len__(self):
"""Returns the number of values in the range"""
x = 0
for i in self:
x += 1
return x
That's a seriously inefficient way to calculate the length
def tuple(self):
"""Returns a tuple representation of the maximum, minimum, and step"""
return (self.maximum, self.start, self.step)
def dict(self):
"""Returns a dictionary representation of the maximum, minimum, and step"""
return {"Max": self.maximum, "Min": self.start, "Step": self.step}
Why? Why would you want either of these functions
def getmaximum(self):
"""Gets the length of the range"""
return self.maximum
def getstart(self):
"""Gets the starting point of the range"""
return self.start
def getstep(self):
"""Gets the step of the range"""
return self.step
Python doesn't need getter functions. Just access attribute directly or via properties.
def getall(self):
"""Returns all range attributes"""
No, it does not.
print("Start:", self.start)
print("Step:", self.step)
print("Stop:", self.maximum)
return 0
What possible purpose would this function serve?
def change_range(self, attr, option, x):
"""Provides options to change attributes of the range"""
What the fried turkey are you trying to do here? The whole function is a complete and total disaster. Do you seriously want this function? Whatever you are trying to do with it, you are doing it wrong.
def shift(self, x):
"""Shifts both the maximum and the start in one function call"""
try:
if self.maximum + x < 0 or self.start + x < 0:
raise SyntaxError
Raise appropriate errors. Whatever the problem is, its not a SyntaxError.
self.maximum += x
self.start += x
self.maximum, self.start = math.ceil(self.maximum), math.ceil(self.start)
except:
print("Error")
NEVER NEVER NEVER NEVER catch errors and just print "Error." Always catch only the specific errors you want to handle, and always provide all the information available about the problem.
def stretch(self, x):
"""Changes both the maximum and the step in one function call"""
try:
if self.maximum + x < 0 or self.step + x < 0:
raise SyntaxError
self.maximum += x
self.step += x
self.maximum, self.step = math.ceil(self.maximum), math.ceil(self.step)
except:
print("Error")
Again, don't do that.
def docs():
"""Documentation for the module"""
print("This module was written for Python 3.2")
print("Text Editors Used: Emacs, Komodo Edit")
print("OS Used: Mac OS X")
Documentation should go into the docstring for the file. Not in a docs function.
This class seems very odd. I suspect that there is probably a much better way of doing whatever you are trying to do. But since you don't tell us what you are trying to do, I can't say for certain.
I mostly agree with Winston Ewert that there's likely a better way. In case there's not, here's something to cut your SLOC way down:
def change_range(self, attr, option, x):
val = getattr(self, attr)
op = { "add" : lambda v: v += x,
"sub" : lambda v: v -= x,
"div" : lambda v: v /= x,
"mult" : lambda v: v *= x }[option]
try:
val = op(val)
except Error as e:
print("Error in change_range %s %s %d: %s" % (attr, option, x, e))
if val <= 0:
raise SyntaxError
setattr(self, attr, math.ceil(val))
It's not an exact workalike (your odd error-checking for values of val is in a slightly different place in some cases) but it's pretty close. Tweak it as needed, of course.