4
\$\begingroup\$

I have written my first code of object oriented Python. Prior to this I have spent a week on learning the concepts and understanding the technique. I would appreciate it somebody reviews this and give suggestion on where I can improve.

I am working through codewars Kata and this code is for the problem. I am copying the problem here so that some one can see what I have solved.

Write a class called User that is used to calculate the amount that a user will progress through a ranking system similar to the one Codewars uses.

Business Rules:

  • A user starts at rank -8 and can progress all the way to 8.

  • There is no 0 (zero) rank. The next rank after -1 is 1.

  • Users will complete activities. These activities also have ranks.

  • Each time the user completes a ranked activity the users rank progress is updated based off of the activity's rank

  • The progress earned from the completed activity is relative to what the user's current rank is compared to the rank of the activity

  • A user's rank progress starts off at zero, each time the progress reaches 100 the user's rank is upgraded to the next level

  • Any remaining progress earned while in the previous rank will be applied towards the next rank's progress (we don't throw any progress away). The exception is if there is no other rank left to progress
    towards (Once you reach rank 8 there is no more progression).

  • A user cannot progress beyond rank 8.

  • The only acceptable range of rank values is -8,-7,-6,-5,-4,-3,-2,-1,1,2,3,4,5,6,7,8. Any other value should raise an error.

The progress is scored like so:

  • Completing an activity that is ranked the same as that of the user's will be worth 3 points

  • Completing an activity that is ranked one ranking lower than the user's will be worth 1 point

  • Any activities completed that are ranking 2 levels or more lower than the user's ranking will be ignored

  • Completing an activity ranked higher than the current user's rank will -accelerate the rank progression. The greater the difference between rankings the more the progression will be increased. The formula is 10 * d * d where d equals the difference in ranking between the activity and the user.

class User():
    rank_vector =[i for i in range(-8,9,1) if ( i!=0)]

    def __init__(self):
        self.rank=-8
        self.progress=0

    def inc_progress(self,kata):
        if kata not in self.rank_vector:
            raise ValueError("Not in the specified Range of features")
        if (self.rank==8):
            progressmeter=0
        elif(self.rank_vector.index(kata) ==self.rank_vector.index(self.rank)):
            progressmeter=self.progress+3
        elif(self.rank_vector.index(kata)==self.rank_vector.index(self.rank)-1):
            progressmeter=self.progress+1
        elif(self.rank_vector.index(kata) <= self.rank_vector.index(self.rank)-2):
            progressmeter=self.progress
        elif(self.rank==-1 and kata==1):
            progressmeter=self.progress+10

        else:
            progressmeter=self.progress+ 10* pow(abs(self.rank_vector.index(kata)-self.rank_vector.index(self.rank)),2)
        progressIndex=list(divmod(progressmeter,100))
        self.progress=progressIndex[1]
        self.rank=self.__updaterank__(progressIndex[0])
        if self.rank==8:
            self.progress=0
        return self.progress


    def __updaterank__(self,level=1):

        if self.rank==8:
            return self.rank
        elif self.rank_vector.index(self.rank)+level > self.rank_vector.index(8):
            self.rank=8
        else:
            self.rank=self.rank_vector[self.rank_vector.index(self.rank)+level]
        return self.rank
\$\endgroup\$
  • \$\begingroup\$ @SirPython Not one to be pedantic, but it's actually a question not an answer. Regardless, very well done Susmita Ghosh! \$\endgroup\$ – Tersosauros Apr 10 '16 at 16:40
3
\$\begingroup\$

On top of what zondo has said you should avoid using parenthesis unnecessarily. There are numerous examples of this:

if (self.rank==8):

can just be:

if self.rank == 8:

also less obvious:

rank_vector =[i for i in range(-8,9,1) if ( i!=0)]

should be:

rank_vector = [i for i in range(-8,9) if i != 0] #Also, no need for the additional "1" parameter.

while we're at it, although you explained it in the description, -8 (nor 8 for what it is worth) isn't the most common of numbers and it seems (without explanation) to be a bit weird.

Maybe somewhere like above rank_vector say:

MIN_RANK = -8
MAX_RANK = 8

And use this when appropriate.

\$\endgroup\$
  • \$\begingroup\$ Grrr. I thought of that about the parentheses, but I was distracted and forgot to mention it. :'| \$\endgroup\$ – zondo Apr 10 '16 at 2:52
  • 1
    \$\begingroup\$ +1 for the C-style constants for the rank min/max values. I think having "magic numbers" in the code (that may change arbitrarily from the domain) is a general code smell. \$\endgroup\$ – Tersosauros Apr 10 '16 at 16:54
2
\$\begingroup\$

The first thing I do when preparing a review is open up PEP 8, the Python style guide. I will refer to it below.

Spacing

From PEP 8

Always surround these binary operators with a single space on either side: assignment (=), augmented assignment (+=,-= etc), comparisons (==,<,>,!=,<>,<=,>=,in,not in,is,is not), Booleans (and,or,not).

Your spacing seems rather strange. You are never using spaces around what is mentioned above, but you do have a case of a space after the open parenthesis but no space before the close parenthesis. While we're at it, that particular space is against PEP 8:

Avoid extraneous whitespace in the following situations:

Immediately inside parentheses, brackets or braces.

Yes: spam(ham[1], {eggs: 2})
No: spam( ham[ 1 ], { eggs: 2 } )
...

You also have no space before + but a space after it; no space before * but a space after it; etc. Whatever you do, (even if you don't follow PEP 8), be consistent. Remember, code is read more times than it is written. It's hard to read code when it doesn't follow recognized guidelines and isn't even consistent.

progressmeter=self.progress+ 10* pow(abs(self.rank_vector.index(kata)-self.rank_vector.index(self.rank)),2)

That's 107 characters not including indentation. PEP 8 says:

Limit all lines to a maximum of 79 characters.

It's a good rule. That line is quite hard to read. What is going on here? I would suggest splitting it up; define variables that make it more obvious what particular parts of the equation are.

Naming

PEP 8 also says:

Method Names and Instance Variables

Use the function naming rules: lowercase with words separated by underscores as necessary to improve readability.

My final comment:

__double_leading_and_trailing_underscore__: "magic" objects or attributes that live in user-controlled namespaces. E.g. __init__ , __import__ or __file__ . Never invent such names; only use them as documented.

Your __updaterank__ method ...

\$\endgroup\$
  • 1
    \$\begingroup\$ Thanks for your valuable feedback . __updaterank__ is a mistake from my part , I wanted to make it private and it should be __updaterank instead . I will keep your suggestions in mind. \$\endgroup\$ – Susmita Ghosh Apr 10 '16 at 2:04
  • \$\begingroup\$ @Zondo Regarding PEP8 section where it talks about variable names. Section "Method Names and Instance Variables" does indeed say to "Use the function naming rules". \$\endgroup\$ – Tersosauros Apr 10 '16 at 16:52
  • 1
    \$\begingroup\$ @Tersosauros: Hah. I didn't catch that about Instance Variables. Thanks. I'll edit. \$\endgroup\$ – zondo Apr 10 '16 at 16:52
1
\$\begingroup\$

Write for clarity of purpose, not clarity of logic

What I mean by that is, instead of writing program structures (i.e. if/else blocks) the way you would in lower-level (often procedural) languages, such as C - make use of more Pythonic ways of expressing your purpose (when/where you can).

Consider the __updaterank__ function (which, as pointed out by @zondo, should be "_updaterank"):

def __updaterank__(self,level=1):

    if self.rank==8:
        return self.rank
    elif self.rank_vector.index(self.rank)+level > self.rank_vector.index(8):
        self.rank=8
    else:
        self.rank=self.rank_vector[self.rank_vector.index(self.rank)+level]
    return self.rank

It has a three-legged if/else, when it could (but shouldn't) be achieved with:

 def _updaterank(self,level=1):
    self.rank = min(MAX_RANK, self.rank_vector[self.rank_vector.index(self.rank)+level)])
    return self.rank

However, this one-liner has lost a lot of clarity^. But, consider the following:

 def _updaterank(self,level=1):
    new_rank_idx = self.rank_vector.index(self.rank)+level)
    self.rank = min(MAX_RANK, self.rank_vector[new_rank_idx])
    return self.rank

^   it also goes over the PEP 8 line limit of 79 chars that @zondo spoke about.

This is clearer, as it tells anyone reading that the (rather awkward to interpret) first line is getting an index of a rank. It also makes it clear we are both setting and then (always) returning the current rank. The use of the built-in min shows readers the logic that had been in the if/else statement, but does this with much more clarity.     (It also has 1/3rd the Cyclomatic Complexity).

I will leave the (similar) optimisation of inc_progress as an exercise for the reader.


Documentation

Though this code was for a , in the future you will want to include documentation strings in functions, etc. (I would assume this is what the blank first line in the body of __updaterank__ was for, although if so it's terribly inconsistent with the other functions :P)

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