1
\$\begingroup\$

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.

asked Apr 28, 2012 at 19:05
\$\endgroup\$
4
  • \$\begingroup\$ Better in what way? Be specific. \$\endgroup\$ Commented 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\$ Commented 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\$ Commented 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\$ Commented Apr 28, 2012 at 20:11

2 Answers 2

5
\$\begingroup\$
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.

answered Apr 28, 2012 at 19:32
\$\endgroup\$
0
1
\$\begingroup\$

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.

answered May 16, 2012 at 6:45
\$\endgroup\$

Your Answer

Draft saved
Draft discarded

Sign up or log in

Sign up using Google
Sign up using Email and Password

Post as a guest

Required, but never shown

Post as a guest

Required, but never shown

By clicking "Post Your Answer", you agree to our terms of service and acknowledge you have read our privacy policy.

Start asking to get answers

Find the answer to your question by asking.

Ask question

Explore related questions

See similar questions with these tags.