Papers, Please - Kata from CodeWars - Python

Question

This is possibly one of the longest katas I've finished so far.

https://www.codewars.com/kata/59d582cafbdd0b7ef90000a0/train/python

I've been working on this one for about an hour a day for about 2 weeks, 90% of the time it's only been debugging. I'm trying to work on my code readability, but also optimizing the code as much as possible.

I'd like to know if YOU can understand something from the code I've written. Would love to hear opinions on how to improve the readability, optimizing some stuff that you notice. I know the code is long, so only check it out if you've got the time.

I had a lot of print statements during debugging, but I've deleted all of them. If you would like the version with all print statements, tell me.

I love constructive criticism, so don't hesitate. Below , I am going to add the task of the project, just in case the link gets deleted or something:

Objective

Your task is to create a constructor function (or class) and a set of instance methods to perform the tasks of the border checkpoint inspection officer. The methods you will need to create are as follow:

Method: receiveBulletin

Each morning you are issued an official bulletin from the Ministry of Admission. This bulletin will provide updates to regulations and procedures and the name of a wanted criminal.

The bulletin is provided in the form of a string. It may include one or more of the following:

• Updates to the list of nations (comma-separated if more than one) whose citizens may enter (begins empty, before the first bulletin):

  example 1: Allow citizens of Obristan
example 2: Deny citizens of Kolechia, Republia

• Updates to required documents

  example 1: Foreigners require access permit
example 2: Citizens of Arstotzka require ID card
example 3: Workers require work pass

• Updates to required vaccinations

  example 1: Citizens of Antegria, Republia, Obristan require polio vaccination
example 2: Entrants no longer require tetanus vaccination

• Update to a currently wanted criminal

  example 1: Wanted by the State: Hubert Popovic

Method: inspect

Each day, a number of entrants line up outside the checkpoint inspection booth to gain passage into Arstotzka. The inspect method will receive an object representing each entrant's set of identifying documents. This object will contain zero or more properties which represent separate documents. Each property will be a string value. These properties may include the following:

passport ID_card (only issued to citizens of Arstotzka) access_permit work_pass grant_of_asylum certificate_of_vaccination diplomatic_authorization 

The inspect method will return a result based on whether the entrant passes or fails inspection:

Conditions for passing inspection

• All required documents are present
• There is no conflicting information across the provided documents
• All documents are current (ie. none have expired) -- a document is considered expired if the expiration date is November 22, 1982 or earlier
• The entrant is not a wanted criminal
• If a certificate_of_vaccination is required and provided, it must list the required vaccination
• If entrant is a foreigner, a grant_of_asylum or diplomatic_authorization are acceptable in lieu of an access_permit. In the case where a diplomatic_authorization is used, it must include Arstotzka as one of the list of nations that can be accessed.

If the entrant passes inspection, the method should return one of the following string values:

• If the entrant is a citizen of Arstotzka: Glory to Arstotzka.
• If the entrant is a foreigner: Cause no trouble.

If the entrant fails the inspection due to expired or missing documents, or their certificate_of_vaccination does not include the necessary vaccinations, return Entry denied: with the reason for denial appended.

Example 1: Entry denied: passport expired.
Example 2: Entry denied: missing required vaccination.
Example 3: Entry denied: missing required access permit.

If the entrant fails the inspection due to mismatching information between documents (causing suspicion of forgery) or if they're a wanted criminal, return Detainment: with the reason for detainment appended.

• If due to information mismatch, include the mismatched item. e.g. Detainment: ID number mismatch.
• If the entrant is a wanted criminal: Detainment: Entrant is a wanted criminal.
• NOTE: One wanted criminal will be specified in each daily bulletin, and must be detained when received for that day only. For example, if an entrant on Day 20 has the same name as a criminal declared on Day 10, they are not to be detained for being a criminal.
• Also, if any of an entrant's identifying documents include the name of that day's wanted criminal (in case of mismatched names across multiple documents), they are assumed to be the wanted criminal.

In some cases, there may be multiple reasons for denying or detaining an entrant. For this exercise, you will only need to provide one reason.

• If the entrant meets the criteria for both entry denial and detainment, priority goes to detaining.
• For example, if they are missing a required document and are also a wanted criminal, then they should be detained instead of turned away.
• In the case where the entrant has mismatching information and is a wanted criminal, detain for being a wanted criminal.

Test Example

bulletin = """Entrants require passport Allow citizens of Arstotzka, Obristan""" inspector = Inspector() inspector.receive_bulletin(bulletin) entrant1 = { "passport": """ID#: GC07D-FU8AR NATION: Arstotzka NAME: Guyovich, Russian DOB: 1933.11.28 SEX: M ISS: East Grestin EXP: 1983.07.10""" } inspector.inspect(entrant1) #=> 'Glory to Arstotzka.' 

Additional Notes

• Inputs will always be valid.
• There are a total of 7 countries: Arstotzka, Antegria, Impor, Kolechia, Obristan, Republia, and United Federation.
• Not every single possible case has been listed in this Description; use the test feedback to help you handle all cases.
• The concept of this kata is derived from the video game of the same name, but it is not meant to be a direct representation of the game.

And here is my solution:

class Inspector: def __init__(self): self.documents_dict = { "passport": [], "ID_card": [], "access_permit": [], "work_pass": [], "grant_of_asylum": [], "certificate_of_vaccination": [], "diplomatic_authorization": [] } self.documents_names =["passport", "ID card", "access permit", "work pass", "grant of asylum", "certificate of vaccination", "diplommatic authorization"] self.vaccinations_dict = {} self.actual_bulletin = { "allowed_nations": [], "denied_nations": [], "required_documents": self.documents_dict, "required_vaccinations": self.vaccinations_dict, "new_criminal": "" } def receive_bulletin(self, bulletin): nations_List = ["Arstotzka", "Antegria", "Impor", "Kolechia", "Obristan", "Republia", "United Federation"] bulletin_lines_List = bulletin.split("\n") def update_allowed_and_denied_nations(): def return_commaless_line(line): if "," in line: line = line.split(" ") for i in range(len(line)): line[i] = line[i].replace(",", "") return " ".join(line) for nation in nations_List: if nation in line: return line def return_line_containing_nations(list): for line in list: for nation in nations_List: if (nation in line and "Allow citizens of " in line) or (nation in line and "Deny citizens of " in line): return line def are_multi_lines_w_nations_Boolean(list): lines_with_nations = 0 for line in list: for nation in nations_List: if nation in line: lines_with_nations +=1 break continue if lines_with_nations > 1: return True else: return False def return_lines_w_nations_List(list): lines = [] for line in list: for nation in nations_List: if (nation in line and "Allow citizens of " in line) or (nation in line and "Deny citizens of " in line): lines.append(line) break return lines def return_allowed_nations_from_line_List(line): allowed_nations = [word for word in line.split(" ") if word in nations_List] if "United Federation" in line and "Allow citizens of " in line: allowed_nations.append("United Federation") return allowed_nations def return_denied_nations_from_line_List(line): denied_nations = [word for word in line.split(" ") if word in nations_List and "Deny citizens of " in line] if "United Federation" in line and "Deny citizens of " in line: denied_nations.append("United Federation") return denied_nations def update_actual_bulletin(): for n in new_allowed_nations_List: if n not in self.actual_bulletin["allowed_nations"]: self.actual_bulletin["allowed_nations"].append(n) for n in new_denied_nations_List: if n in self.actual_bulletin["allowed_nations"]: self.actual_bulletin["allowed_nations"].remove(n) new_allowed_nations_List = [] new_denied_nations_List = [] if are_multi_lines_w_nations_Boolean(bulletin_lines_List): multi_lines_w_nations_List = return_lines_w_nations_List(bulletin_lines_List) for i in range(len(multi_lines_w_nations_List)): multi_lines_w_nations_List[i] = return_commaless_line(multi_lines_w_nations_List[i]) single_nations_line_list = [multi_lines_w_nations_List[i] for i in range(len(multi_lines_w_nations_List))] for line in single_nations_line_list: if "Allow citizens of " in line: new_allowed_nations_List.extend(return_allowed_nations_from_line_List(line)) elif "Deny citizens of " in line: new_denied_nations_List.extend(return_denied_nations_from_line_List(line)) update_actual_bulletin() else: if return_lines_w_nations_List(bulletin_lines_List) != []: single_nations_line = return_commaless_line(return_line_containing_nations(bulletin_lines_List)) if "Allow citizens of " in single_nations_line: new_allowed_nations_List.extend(return_allowed_nations_from_line_List(single_nations_line)) elif "Deny citizens of " in single_nations_line: new_denied_nations_List.extend(return_denied_nations_from_line_List(single_nations_line)) update_actual_bulletin() def update_required_documents(): documents_List = \ ["passport", "ID card", "access permit", "work pass", "grant of asylum", "certificate of vaccination", "diplomatic authorization" ] underlined_documents_List = \ ["passport", "ID_card", "access_permit", "work_pass", "grant_of_asylum", "certificate_of_vaccination", "diplomatic_authorization" ] def update_documents_Dict(): for line in bulletin_lines_List: if "Workers require work pass" in line: self.documents_dict["work_pass"].append("Yes") if "Entrants require " in line: for document in documents_List: if "Entrants require " + document in line: self.documents_dict[underlined_documents_List[documents_List.index(document)]] = nations_List continue if "Foreigners require " in line: for document in documents_List: if "Foreigners require " + document in line: self.documents_dict[underlined_documents_List[documents_List.index(document)]] = ["Antegria", "Impor", "Kolechia", "Obristan", "Republia", "United Federation"] if "Citizens of " in line: if "vaccination" not in line: for nation in nations_List: for document in documents_List: if "Citizens of " + nation + " require " + document in line: self.documents_dict[underlined_documents_List[documents_List.index(document)]].append(nation) update_documents_Dict() def update_required_vaccinations(): vaccine_Dict = {} for i, line in enumerate(bulletin_lines_List): if "vaccination" in line: line_List = line.split(" ") def return_commaless_line(list): return [s.replace (",","") for s in list] line_List = return_commaless_line(line_List) if "United" in line_List: # Because "United Federation" has a space between line_List.remove("United") line_List.remove("Federation") line_List.append("United Federation") vaccine_name_List = [w for w in line_List if line_List.index(w) > line_List.index("require") and line_List.index(w) < line_List.index("vaccination")] vaccine_name = " ".join(vaccine_name_List) vaccine_Dict[i] = vaccine_name def bulletin_add_remove_vaccinations(): if "Entrants" in line_List: # Entrants meaning all nations_List if "no" in line_List: self.vaccinations_dict.pop(vaccine_Dict[i]) # Remove all nations_List else: self.vaccinations_dict[vaccine_Dict[i]] = nations_List # Add all nations_List elif "Foreigners" in line_List: # Foreigners meaning all nations_List except Arstotzka if "no" in line_List: self.vaccinations_dict[vaccine_Dict[i]] = [x for x in self.vaccinations_dict[vaccine_Dict[i]] # Remove all nations_List except Arstotzka if x not in ["Antegria", "Impor", "Kolechia", "Obristan", "Republia", "United Federation"]] else: self.vaccinations_dict[vaccine_Dict[i]] = ["Antegria", "Impor", "Kolechia", # Add all nations_List except Arstotzka "Obristan", "Republia", "United Federation"] elif "Citizens of " in line_List: # Followed by specific nations_List if "no" in line_List: self.vaccinations_dict[vaccine_Dict[i]] = [nation for nation in self.vaccinations_dict[vaccine_Dict[i]] if nation not in line_List] else: self.vaccinations_dict[vaccine_Dict[i]] = [nation for nation in nations_List if nation in line_List] bulletin_add_remove_vaccinations() def update_new_criminal(): for line in bulletin_lines_List: if "Wanted by the State:" in line: line_List = line.split(" ") wanted = line_List[line_List.index("State:") + 1] + " " + line_List[line_List.index("State:") + 2] self.actual_bulletin["new_criminal"] = wanted update_allowed_and_denied_nations() update_required_documents() update_required_vaccinations() update_new_criminal() def inspect(self, entrant): nations_List = ["Arstotzka", "Antegria", "Impor", "Kolechia", "Obristan", "Republia", "United Federation"] documents_List = \ ["passport", "ID card", "access permit", "work pass", "grant of asylum", "certificate of vaccination", "diplomatic authorization"] underlined_documents_List = \ ["passport", "ID_card", "access_permit", "work_pass", "grant_of_asylum", "certificate_of_vaccination", "diplomatic_authorization"] def returns_nation(): for document in entrant.values(): for nation in nations_List: if nation in document: return nation def check_nation(): if returns_nation() == "Arstotzka": return if returns_nation() not in self.actual_bulletin["allowed_nations"]: return "Entry denied: citizen of banned nation." def check_missing_documents(): if entrant == {}: # Missing all documents return "Entry denied: missing required passport." documents_req_for_nation_List = [doc for doc in self.documents_dict if returns_nation() in self.documents_dict[doc]] vaccine_certificates_req_for_nation_List = [vac_doc for vac_doc in self.vaccinations_dict if returns_nation() in self.vaccinations_dict[vac_doc]] if documents_req_for_nation_List == []: for doc in self.documents_dict.keys(): if self.documents_dict[doc] == nations_List: documents_req_for_nation_List.append(doc) for document in documents_req_for_nation_List: if document not in entrant.keys(): if document == "access_permit": if ("grant_of_asylum" in entrant.keys()): return if ("diplomatic_authorization" in entrant.keys()): if "Arstotzka" in entrant.get("diplomatic_authorization"): return else: return "Entry denied: invalid diplomatic authorization." return "Entry denied: missing required " + documents_List[underlined_documents_List.index(document)] + "." if len(vaccine_certificates_req_for_nation_List) > 0: if "certificate_of_vaccination" not in entrant.keys(): return "Entry denied: missing required certificate of vaccination." if self.documents_dict["work_pass"] != []: for i in range(len(entrant.values())): if "WORK" in list(entrant.values())[i]: if "work_pass" not in entrant.keys(): return "Entry denied: missing required work pass." def check_mismatching_information(): def check_mismatching_ID(): document_and_ID_Dict = {} documents_and_keys_List = [key for key in entrant.keys()] for i, value in enumerate(entrant.values()): for line in value.split("\n"): if "ID#:" in line: ID = line.split(" ") for x in ID: if "ID#:" in x: ID.pop(ID.index(x)) document_and_ID_Dict[documents_and_keys_List[i]] = ID expected_value = next(iter(document_and_ID_Dict.values())) all_IDs_equal = all(value == expected_value for value in document_and_ID_Dict.values()) if not all_IDs_equal: return "Detainment: ID number mismatch." if "Detainment:" in str(check_mismatching_ID()): return check_mismatching_ID() def check_mismatching_name(): document_and_name_Dict = {} documents_and_keys_List = [key for key in entrant.keys()] for i, value in enumerate(entrant.values()): for line in value.split("\n"): if "NAME:" in line: name_line = line.split(" ") for x in name_line: if "NAME:" in x: name_line.pop(name_line.index(x)) document_and_name_Dict[documents_and_keys_List[i]] = name_line expected_value = next(iter(document_and_name_Dict.values())) all_names_equal = all(value == expected_value for value in document_and_name_Dict.values()) if not all_names_equal: return "Detainment: name mismatch." if "Detainment:" in str(check_mismatching_name()): return check_mismatching_name() def check_mismatching_nation(): document_and_nation_Dict = {} documents_and_keys_List = [key for key in entrant.keys()] for i, value in enumerate(entrant.values()): for line in value.split("\n"): if "NATION:" in line: nation_line = line.split(" ") for x in nation_line: if "NATION:" in x: nation_line.pop(nation_line.index(x)) document_and_nation_Dict[documents_and_keys_List[i]] = nation_line try: expected_value = next(iter(document_and_nation_Dict.values())) all_nations_equal = all(value == expected_value for value in document_and_nation_Dict.values()) if not all_nations_equal: return "Detainment: nationality mismatch." except StopIteration: pass if "Detainment:" in str(check_mismatching_nation()): return check_mismatching_nation() def check_mismatching_DOB(): # DOB - Date of Birth document_and_dob_Dict = {} for i, value in enumerate(entrant.values()): for line in value.split("\n"): if "DOB:" in line: DOB_line = line.split(" ") for x in DOB_line: if "DOB:" in x: DOB_line.pop(DOB_line.index(x)) document_and_dob_Dict[list(entrant.keys())[i]] = DOB_line try: expected_value = next(iter(document_and_dob_Dict.values())) except Exception: pass all_dobs_equal = all(value == expected_value for value in document_and_dob_Dict.values()) if not all_dobs_equal: return "Detainment: date of birth mismatch." if "Detainment:" in str(check_mismatching_DOB()): return check_mismatching_DOB() def check_missing_vaccination(): nation = returns_nation() vaccines_req_for_nation_Dict = [vac for vac in self.vaccinations_dict if nation in self.vaccinations_dict[vac]] for vac in vaccines_req_for_nation_Dict: for i in range(len(list(entrant.values()))): if vac not in list(entrant.values())[i]: continue else: return return "Entry denied: missing required vaccination." def check_documents_expiration_date(): document_and_exp_date_Dict = {} document_and_key_List = [key for key in entrant.keys()] for i, value in enumerate(entrant.values()): for line in value.split("\n"): if "EXP:" in line: date = line.split(" ") for x in date: if "EXP:" in x: date.pop(date.index(x)) date_list = date[0].split(".") document_and_exp_date_Dict[document_and_key_List[i]] = date_list for i in range(len(document_and_key_List)): if (document_and_key_List[i] != "diplomatic_authorization" and document_and_key_List[i] != "ID_card") and document_and_key_List[i] != "certificate_of_vaccination": if document_and_exp_date_Dict[document_and_key_List[i]][0] < "1982": return "Entry denied: " + documents_List[underlined_documents_List.index(document_and_key_List[i])]+ " expired." elif document_and_exp_date_Dict[document_and_key_List[i]][0] == "1982": if document_and_exp_date_Dict[document_and_key_List[i]][1] < "11": return "Entry denied: " + documents_List[underlined_documents_List.index(document_and_key_List[i])]+ " expired." elif document_and_exp_date_Dict[document_and_key_List[i]][1] == "11": if document_and_exp_date_Dict[document_and_key_List[i]][2] <= "22": return "Entry denied: " + documents_List[underlined_documents_List.index(document_and_key_List[i])]+ " expired." def return_criminal_name(): for value in entrant.values(): for line in value.split("\n"): if "NAME:" in line: name = line.split(" ") name.pop(0) for n in range(len(name)): if "," in name[n]: name[n] = name[n].replace(",", "") return name[1] + " " + name[0] if len(entrant.keys()) > 1: if "Detainment:" in str(check_mismatching_information()): return check_mismatching_information() if return_criminal_name() == self.actual_bulletin["new_criminal"]: return "Detainment: Entrant is a wanted criminal." if "Entry denied: " in str(check_missing_documents()): return check_missing_documents() if "Entry denied: " in str(check_documents_expiration_date()): return check_documents_expiration_date() if "Entry denied: " in str(check_missing_vaccination()): return check_missing_vaccination() if "Entry denied: " in str(check_nation()): return "Entry denied: citizen of banned nation." if returns_nation() == "Arstotzka": return "Glory to Arstotzka." else: return "Cause no trouble." 

Answer 1

Inspector! Why did you allow Gregory Arstotzkaya, a Kolechian spy, into our glorious nation? Or Ilyana Dbrova, whose documents were perfect forgeries, except for a gender mismatch in her ID card? And why did you fail to apprehend Karl von Oskowitz?

• Treating documents as a single piece of text and just checking whether they contain certain words is problematic. Instead, parse each document into a dictionary, which makes it easy to look at relevant fields only. This also simplifies your code, because you no longer need to have parsing code all over the place.
• Similarly, using string comparisons for dates is not a good idea. Parse dates into a datetime object and compare those instead.
• You're making things more difficult for yourself by splitting on spaces rather than commas:
  • Today it's just a single hack for the United Federation, tomorrow Antegria splits up into North Antegria and the Republic of South Antegria...
  • Why hard-code the names of any country (except for the motherland, of course)? If someone claims to be from Nonexististan, well, non-existing countries aren't on our whitelist, so they'll be rejected anyway. And why should you ignore a bulletin that tells you to allow people from a newly formed country?
  • If you're having difficulties isolating specific parts of a sentence, for simple cases you could use slicing: string[start:end] gives you a new string that only contains the specified part of the original string. Negative offsets count from the end of the string, left-out offsets default to the start/end of the string.
• Parsing bulletins would be much easier with the help of a few well-chosen regular expressions. Consider creating a list of pattern-function tuples, such as ('Allow citizens of (?P<countries>.+)', allow_citizens), where allow_citizens is a function that takes a match object. You can fetch the countries with match.groups('countries'), which can then easily be split on commas. Be sure to strip() leading and trailing spaces from each individual country name. Now, for each line in a bulletin, your receive_bulletin function would walk through this list, calling re.match for each pattern until it finds a match, which it passes on to the associated function. To handle the similarities between document and vaccination instructions, you could order the patterns from most to least specific, or use look-behind assertions, or perform additional checks in the associated function.
• It's probably easier to store document and vaccination requirements per country, rather than per document type. That means that once you know where someone is from, you immediately know which documents are required, instead of having to check every document type for the person's country.
• For requirements, I would use a defaultdict, from the collections module, with the set function as default factory. This means you don't need to worry about initialization: you can just do required_documents[country].add(document) without having to check whether required_documents even contains that country key.
• In inspect, you're calling a lot of functions twice. First to see if the entrant should be detained or rejected, then again for the actual response. That's a waste of work. Just call these functions once and store their result in a local variable, then check that variable and return it if necessary.

Most nested functions are reasonably named, which makes it easy to get an idea of what your code does. However, a lot of them depend on local state in their 'parent' function, which makes it difficult to understand how they work. There's also a fair amount of (almost) duplicated code and special-case handling that could be removed with more robust general-case code.

A function that takes all of its input via arguments, and returns its output, without modifying any other state - a 'pure' function - is easier to understand and easier to (re)use. For example, compare check_documents_expiration_date() with is_document_expired(document, current_date): with the former, it's not immediately clear which documents it inspects, or against which expiry date. The latter clearly checks the given document against the given date. It's doing less, but can be used as a building block for other functions, or in small expressions like expired_documents = [doc for doc in documents if is_document_expired(doc, current_date)].

Answer 2

Since this is a lot of code, this answer might expand over time or leave parts to other capable members of this community.

---

Style

Your overall style is quite good. You seem to follow most of the guidelines presented in the official Style Guide for Python Code (the infamous PEP8). However, there are a few things that might need a little tweaking.

The thing that caught my attention in the first place where all those _List and _Boolean suffixes on variable an function names. They are very, very uncommon to be seen in any serious Python code. Usually you would choose your variable name to be as explicit as possible about its content, but care less about the type. If you really wanted to use "typing", Python 3 now offers so called type hints as a "standard" way to convey type information to users and tools. So instead of def are_multi_lines_w_nations_Boolean(list): you would have def are_multi_lines_w_nations(list_: list) -> bool:. The keen observer will see that I also changed the parameter name from list to list_. This is because list is the typename of Python's built-in list type (think []). There are other names like this that should be avoided, e.g input, file, and open to name a few.

In addition, your code code use some documentation. There is a section about these so called documentation strings or docstrings for short in the style guide. They are basically short descriptions of what your functions do """enclosed in triple quotes""" immediately after def function_name(...): or class ClassName:. Especially if the code becomes even more complex you will be grateful for any hint your IDE can give you about the code you have written a month ago. Following the style as recommended in the Style Guide (and basically found everywhere in Python code) will allow not just Python's built-in help(...) to pick up your doc, but also most of the IDEs. This documentation might also be used to convey type information to the user.

Another stylistic choice to take is how to format long lists and dictionaries in a consistent way. You have several:

The long one:

nations_List = ["Arstotzka", "Antegria", "Impor", "Kolechia", "Obristan", "Republia", "United Federation"] 

Multiline, starting on the same line with the first element:

self.documents_names = ["passport", "ID card", "access permit", "work pass", ...] 

Multiline, starting the overall list on the next line:

documents_List = \ ["passport", "ID card", "access permit", ...] 

Multiline, opening and closing brackets on seperate lines:

self.documents_dict = { "passport": [], "ID_card": [], "access_permit": [], ... } 

However you choose, it's best to stick with one of them. Over time I have come to the conclusion that I like the last one best.

The good news is: there are a lot of tools out there that can help you to keep a consistent style even in larger projects. They start at tools like flake8 and pylint which can basically perform style checking on your code. The next step are tools like yapf or black that can even auto-format your code while you write it. pylint or more specialised tools like mypy can also perform static code analysis to judge program complexity and look e.g. for type hints and the like.

---

The code

The code itself has some serious tendency to do deep nesting. IIRC, the most extreme case was like ten (10!) levels deep. Your class also has a significant number of lines of code (around 600). Where about ~220 of them are the taken up by the parser of the daily bulletin. It might be worth to consider moving the whole bulletin parsing ouf of Inspector into a separate class which is then used by the Inspector.

Other than that, there is quite a bit of code repetition. Especially the list of nations is repeated a lot. You should put them into "constants" right at th beginning of the file and use them where appropriate, e.g.

OUR_GREAT_NATION = "Arstotzka" FOREIGN_NATIONS = ( "Antegria", "Impor", "Kolechia", "Obristan", "Republia", "United Federation" ) ALL_NATIONS = (OUR_GREAT_NATION, ) + FOREIGN_NATIONS # ... later for nation in ALL_NATIONS: if nation in line: return line 

Note: I used a tuple because they are immutable. This prevents you from doing things like ALL_NATIONS[0] = "Foo".

---

That's all for now. Maybe I can spare some time tomorrow to talk a little bit more about some specific details.

Answer 3

In addition to the excellent points AlexV said above, her are a few more observations:

There is quite a bit of repetition in the bulletin parsing code code. For example, a check is made to see if "Accept citizens of" if in a line in four (4) different places. Each of the update_xxx functions loops over all the lines in the bulletin and checks to see if the line contains a certain word or phrase (e.g. 'Accept' or 'Wanted'). If it does match, then some action is taken. Because there are only a few different kinds of bulleting lines, this can be simplified to only loop over the lines once, like so:

def receiveBulletin(bulletin): for line in bulletin.splitlines(): if 'Accept' in line: update_accepted_countries(line) elif 'Deny' in line: update_denied_countries(line) ... etc ... 

Instead of iterating over the words in a line to see if they match a country name, it might be easier to iterate over the country names and see if they are in the line. That way you don't have to handle 'United Federation' specially.

Choosing the right data structure can make a big difference in how simple or complex the code is. For example, because self.actual_bulletin["allowed_nations"] is a list, you need to check to see if a nation is in the list before you add and delete it. If you used a set() instead, that check would not be necessary, because set.add() and set.discard() will take care of that. Similarly, if vaccines_required_for_nation and entrant_has_vaccines are both sets, then vaccines_required_for_nation <= entrant_has_vaccines is False unless the entrant has the required vaccines.

Answer 4

This is a mere supplement to the other answers.

First, there are several places in the code where you stack two if-statements on top of each other, because you want to check two different conditions before proceeding with an operation:

 if "Citizens of " in line: if "vaccination" not in line: for nation in nations_List: 

In these cases it would be better to have a single if-statement. This is more typical and makes your intentions clearer to future readers. (If I saw a double-if-stack out in the wild, I would immediately begin looking for the else clause that goes with the second if, because why else would it be written like that?)

 if "Citizens of " in line and "vaccination" not in line: for nation in nations_List: 

Second, some of your lines are very long (179 characters at the longest). AlexV's answer discussed the problem of formatting lists and dictionaries, but other parts of your code are even longer:

 for i in range(len(document_and_key_List)): if (document_and_key_List[i] != "diplomatic_authorization" and document_and_key_List[i] != "ID_card") and document_and_key_List[i] != "certificate_of_vaccination": if document_and_exp_date_Dict[document_and_key_List[i]][0] < "1982": return "Entry denied: " + documents_List[underlined_documents_List.index(document_and_key_List[i])]+ " expired." 

This is typically considered poor style. It increases the odds that someone will have to scroll both vertically and horizontally to read your code, which is slightly annoying and certainly doesn't aid understanding.

The PEP 8 guidelines recommend limiting lines to 79 characters, maybe 99, and provide some guidance on where and how to break lines. Limiting the amount of nesting in your code will help too. You can even shorten some of your variable names (though you don't want to sacrifice descriptive power).

 for i in range(len(doc_names)): if (doc_names[i] != "diplomatic_authorization" and doc_names[i] != "ID_card" and doc_names[i] != "certificate_of_vaccination"): if doc_exp_date[doc_names[i]][0] < "1982": return ("Entry denied: " + docs[underlined_docs.index(doc_names[i])] + " expired.")