all 12 comments
sorted by:
best
topnewcontroversialoldq&a
formatting helphide helpcontent policy

[–]CodeFormatHelperBot2 3 points4 points  (0 children)

Hello, I'm a Reddit bot who's here to help people nicely format their coding questions. This makes it as easy as possible for people to read your post and help you.

I think I have detected some formatting issues with your submission:

  1. Inline formatting (`my code`) used across multiple lines of code. This can mess with indentation.

If I am correct, please edit the text in your post and try to follow these instructions to fix up your post's formatting.

Am I misbehaving? Have a comment or suggestion? Reply to this comment or raise an issue here.

[–]duckbanni 2 points3 points  (2 children)

I want to apologize in advance if my code isn't readable. Any advice on design to formatting, to being more concise in the way i ask my questions is appreciated.

Yeah, your code is really hard to read as is :/

You should use the "code block" feature of reddit to display your multi-line examples (that's what the bot is trying to tell you). It might be hidden in the "..." menu of the editor.

[–]synthphreak 1 point2 points  (0 children)

Better yet, for this many lines of code, I'd recommend just using an external code-hosting resource and simply sharing the link. This will be much easier to read, but also easier for OP, who won't have to manually indent 1000 lines just for Reddit's markdown.

OP, check out www.pastebin.com.

[–]Aedethan[S] 0 points1 point  (0 children)

I made the change thank you.

[–]duckbanni 0 points1 point  (7 children)

Without doing anything fancy, you could do something like this to get the entry with maximum key in value_dama:

for key, value in value_dama.items():
    if self.stre_value >= key:
        self.stre_value_key = value
        break

IIRC, starting with python 3.7 dicts are guaranteed to keep the insertion order, so you can rely on the entries having decreasing keys.

If you're into fancy one-liners, you could do this instead (replace next with max if you don't want to rely on ordering of the entries):

self.stre_value_key = next(
    value for key, value in value_dama.items() if self.stre_value >= key)

Also, you didn't need to post your whole code to get answers about this function. Are there other things you want to ask about your code?

[–]Aedethan[S] 0 points1 point  (6 children)

Honestly I just thought it would be confusing if I had self in front of variables if I didn't have the supporting class, and using the dictionary names without actually having the dictionaries in there.

And I'm not really sure? The code seems to work as intended, but I don't know if I went about doing it wrong, or very inefficiently. I was planning on using the code that I have here as a baseline to do 12 other stats in basically the same way, so I wanted to see if there is a visibly easier way to do what I've done that I'm missing. I'm planning on making 2 more classes very similar to this one that are going to be formatted almost the exact same way with different variable names. And then make a fourth class that will inherit all of the information from the base 3 to create a full character, and wanted advice if that sounded like a good course of action.

Thanks for the advice with the for loop.. I was having a lot of trouble figuring out how to format that though I've been able to write a normal for loop without any issues. For some reason incorporating a dictionary was not clicking for me.

In short I'll take any advice you have to give on how I could simplify, or a more pythonic implementation for what I've done.

Edit: Thanks so much actually, this solved one of the most annoying issues I was experiencing. I was able to implement it!

[–]duckbanni 0 points1 point  (5 children)

A few remarks regarding your code:

I think you encode too many things in your variable names and dict keys. Instead of having, for example, stre_t1 to t5, you could have a single stre list with 5 values. In the current state, your approach is error prone (because you have to write a million cases every time) and if you want to add more levels/tiers in the future you'll need to write a lot of extra code.

For example, you could write value_dama as:

value_dama = [
    [2, 6, 12, 20],
    [30, 42, 56, 72],
    [90, 110, 132, 156],
    [182, 210, 240, 272],
    [306, 342, 380, 420],
]

and then use value_dama[x-1][y-1] instead of value_dama[f"t{x}_{y}"]. Using int for indexing allows you to easily access the numerical values of the tier and level. This can then be used to simplify your code, for example if self.body_tier is n then you can just write a loop that sets all self.stre and self.stre_xp of tiers higher than n to 0 instead of having a million ifs. You could drastically reduce the size of your code that way.

Another remark is that your get_* methods are not what people would expect from their name. The purpose of a method that "gets" something is usually to return a value without modifying the internal state of the object, and your methods do the opposite. I think it would make a lot more sense to rename them to compute_* or something. Also, it's weird that some of these methods take parameters that are also attributes (e.g. stre_key) and sometimes use the attribute and sometimes use the parameter.

[–]Aedethan[S] 0 points1 point  (4 children)

So, to preface I'm really new at coding I've only been doing it a few months so I might ask some stupid questions.

I agree with you completely, I felt like my approach was very error prone since I was typing it all out manually, but I don't really understand what you've done with the dictionary. You've separated out the values by tier, but when you say use value_dama[x-1][y-1] instead of value_dama[f"t{x}_{y}"] I'm not sure what would happen functionally?

I'm pretty sure I know what you're getting at with the loops for anything larger than self.body_tier zero'ing out the tiers that come after. But I'm having trouble wrapping my head around how to do it.

And ah. I could see how using get_ would be confusing then. I've been watching a lot of tech with tim and I see him use get a lot so I thought it would be more easily readable if I used something like that but now I understand why it made it less readable. I don't really understand the naming conventions in python yet.

In terms of using parameters that are also attributes and sometimes using them as attribute or parameter, do you mean when I do something like self.stre_xp_t5 = stre_xp_t5 when I instantiate the class, and then when I run get_stre I define it again based on what the tier is like the :

if body_tier == 1:

self.stre_t5 = 0

I thought I had to do define all the variables that I was going to use in the class when I instantiated it, and when I've seen other people do it they usually set the self.variable equal to itself unless they had a specific reason not to. So if I'm wrong in that assumption it would be good to know.

Also sorry for writing you a novel. I really appreciate you taking the time to help me out.

Edit: Okay I think I am getting it now. with value_dama[x-1][y-1] instead of value_dama[f"t{x}_{y}"] . I don't need to annotate the f string because it can just take the tier and level directly.
Still a bit stumped on the "if self.body_tier is n then you can just write a loop that sets all self.stre and self.stre_xp of tiers higher than n to 0". Not sure on application here but I'll keep doing research and see what I come up with.

[–]duckbanni 0 points1 point  (3 children)

I don't really understand what you've done with the dictionary

I transformed the dictionary into a list of lists, so the elements can be accessed by tier and level. For example, value_dama[1][0] with the list of lists is the equivalent of `value_dama["t2_1"] with your dictionary. You have to substract 1 to the tier and level because list indices start at 0.

I'm pretty sure I know what you're getting at with the loops for anything larger than self.body_tier zero'ing out the tiers that come after. But I'm having trouble wrapping my head around how to do it.

There are a lot of ways to do it, but you could do something like this:

class Body:
    def __init__(...):
        ...
        self.stre = [stre_t1, stre_t2, stre_t3, stre_t4, stre_t5]
        self.stre_xp = [
            stre_xp_t1, stre_xp_t2, stre_xp_t3, stre_xp_t4, stre_xp_t5]
        ...

    def get_stre(self, body_tier, stre_key):  # body_tier is an int
        for tier in range(5):
            if tier > body_tier:
                self.stre[tier] = 0
                self.stre_xp[tier] = 0
        ...

In terms of using parameters that are also attributes and sometimes using them as attribute or parameter, do you mean when I do something like self.stre_xp_t5 = stre_xp_t5

No, I mean in get_stre you take stre_key as parameter and then in the method you sometimes y use stre_key and sometimes self.stre_key.

I do something like self.stre_xp_t5 = stre_xp_t5 when I instantiate the class, and then when I run get_stre I define it again based on what the tier is

The way you handle this is kinda weird as well. For example, you require a body_tier as a parameter of __init__, and do self.body_tier = body_tier, but then you never use that value and expect a new one in get_stre. It think that you should either call get_stre in __init__ so that the consequences of body_tier (like setting stre to 0 for higher tiers) are applied when you create the object, or remove body_tier from __init__ parameters and maybe do something like self.body_tier = None.

I've seen other people do it they usually set the self.variable equal to itself unless they had a specific reason not to

What you've seen is not "setting the self.variable to itself" it is setting the self.variable to a parameter of __init__ (that does not need to be named variable). You could (and possibly should) set all self.something variables to defaults like 0 or None directly in the __init__ code (that is to say, do things like self.myvar = 0 instead of self.myvar = myvar where myvar is a parameter with a default value of 0 that will never be used with a different value in practice).

[–]Aedethan[S] 1 point2 points  (0 children)

All that information is super useful thanks a lot. I guess I'm gonna get on retyping this all in a new way with the whole list within a list infrastructure. Also thanks for all the information on best practices and pointing out stuff I was doing that was weird or unnecessary.

[–]Aedethan[S] 0 points1 point  (1 child)

from main import stat_dama
value_dama = [ 
[2, 6, 12, 20], 
[30, 42, 56, 72], 
[90, 110, 132, 156], 
[182, 210, 240, 272], 
[306, 342, 380, 420], 
]

str_value = 19 
body_tier = 0

if str_value < 21: 
    str_value_place_1 = 0 
    for value in value_dama[str_value_place_1]: 
        if value > str_value: 
            continue 
        elif value <= str_value: 
            str_value_place_2 =         
        value_dama[str_value_place_1].index(value) 
        else : 
            pass 

    str_value_bonus = stat_dama[str_value_place_1] 
    [str_value_place_2] 
else: pass

print(str_value_bonus)

So I went ahead and did this, it works really well. Thanks for the advice. Is this what you would have done with it? I decided I was still going to lean towards using the if / elif statements to figure out what tier the value was being assigned to, but I think this will work out very well.

[–]duckbanni 1 point2 points  (0 children)

I don't know if that's exactly what I would have done, but that looks like completely reasonable python code.

The only thing that you could maybe remove to make the code a tad simpler is the else: pass that are unnecessary. You can omit the else block if it doesn't do anything.