Skip to content

Conversation

@cadenforrest
Copy link
Collaborator

1st pass at making a query generator

Comment on lines +16 to +17
print(opendota.make_query("Bloodseeker", "Crystal Maiden"))
assert type(opendota.make_query("Bloodseeker", "Crystal Maiden")) is dict
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this will make two separate queries to opendota, eating away at our cap. if you're gonna print, at least assign to an intermediary variable.

also, are there stronger assertions we can make beyond "it's a list"?

Comment on lines +11 to +12
def generate_query(*heroes):
"""generates an SQL query where hero1+hero2 is vs hero3+hero4"""
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I want you to get in the habit of creating more descriptive function names. Conceivably in this project we will be generating many types of queries. What makes this one THE query?

How about generate_laning_matchups_query?

Comment on lines +20 to +34
i = 0 #leave me be sir
for data in heroes:
for hero in data:
if i == 0:
select = select + f" player_matches.hero_id AS hero1, \n"
else:
select = select + f"player_matches{i}.hero_id AS hero{i+1}, \n"
i = i+1
i = 0
for hero in data:
if i == 0:
select = select + f"player_matches.player_slot AS ps1, \n"
else:
select = select + f"player_matches{i}.player_slot AS ps{i+1}, \n"
i = i+1
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks like this is harder than it needs to be.

How about something like

for i, hero in enumerate(heroes):
    select += f"player_matches{i}.hero_id AS hero{i}\n"
    select += f"player_matches{i}.player_slot AS ps{i}, \n"

Comment on lines +46 to +48
if i == 0:
i = i+1
continue
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

seems like this shouldn't be necessary

how about something like

for i, hero in enumerate(heroes):
    if i==0:
        continue
    else:
        join +=...
        join += ...

join = join + f"ON player_matches.match_id = player_matches{i}.match_id\n"
i = i+1
join = join + f"JOIN matches\n"
join = join + f"ON player_matches.match_id = matches.match_id\n"
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

if you implement my suggestion this has to become player_matches1

Comment on lines +79 to +93
for data in heroes:
for hero in data:
if i == 0:
i = i+1
continue
elif i == 1:
teams = teams + f"AND abs(player_matches.player_slot - player_matches1.player_slot) < 6\n"
i = i+1
elif i == 2:
teams = teams + f"AND abs(player_matches.player_slot - player_matches2.player_slot) > 123\n"
i = i+1
elif i == 3:
teams = teams + f"AND abs(player_matches2.player_slot - player_matches3.player_slot) < 6\n"
teams = teams + f"AND abs(player_matches1.player_slot - player_matches3.player_slot) > 123\n"
i = i+1
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

i think we say if it's the first hero in team A, no teams clause. if the hero is >1 item in team A, add same team clause. if in team B, add other team clause, if that makes sense.

Comment on lines +62 to +69
for data in heroes:
for hero in data:
if i == 0:
where = where + f" player_matches.hero_id = {get_hero_id(hero)}\n"
i = i+1
else:
where = where + f"AND player_matches{i}.hero_id = {get_hero_id(hero)}\n"
i = i+1
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

only difference is player_matches vs player_matches{i}, just make them all player_matches{i}

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants