6
\$\begingroup\$

I'm learning object-oriented programming in Python. I've created a class RepositoryMaker that contains all methods required to create a Git repository. The idea is to be able to create a single instance of this class with a constructor that only requires pertinent info to create a new repo.

My code looks like this:

class RepositoryMaker:
    """Creates new Azure DevOps repositories"""
    def __init__(self, project_name, repo_name):
        self.project_name = project_name
        self.repo_name = repo_name
        with open("./repo_request_body.json", "r") as file:
            self.data = json.load(file)

    def get_project_id(self):
        """Returns the Azure DevOps project ID for a project name"""
        _request_object = make_request(
            "_apis/projects/",
            self.project_name,
            "?api-version=5.1-preview.1",
            request_method = "get"
            )
        return _request_object.get("id")

    def update_project_id(self):
        """Updates JSON dictionary with new ID"""
        new_id = self.get_project_id()
        self.data["project"]["id"] = new_id

    def update_repo_name(self):
        """Updates JSON dictionary with new repo name"""
        self.data["name"] = self.repo_name
        return self.data

    json_content_type = {
        'Content-Type': 'application/json',
        'Authorization': 'Basic XXXXXXXXXXXXXXXX'
        }

    def create_repository(self):
        """Creates a new repo under the specified project"""
        self.update_project_id()
        repo_request_body = self.update_repo_name()
        make_request(
            self.project_name,
            "/_apis/git/repositories",
            "?api-version=5.1-preview.1",
            request_method = "post",
            data = json.dumps(repo_request_body),
            headers = self.json_content_type
        )


x = RepositoryMaker(project_name, repo_name)
x.create_repository()

My concern is chaining together class methods internally within the class. For example, create_repository() is calling update_project_id() which is calling get_project_ID().

Is this convention acceptable within Python? Is there a better way to group together a set of related functions than using a class?

| improve this question | | | | |
\$\endgroup\$
  • 1
    \$\begingroup\$ I don't work with azure repos, but what if your get_project_id method returned empty id (for non-existent project name)? \$\endgroup\$ – RomanPerekhrest Mar 2 at 15:37
  • \$\begingroup\$ That's a helpful observation, I'll add validation to fix that. \$\endgroup\$ – pirateofebay Mar 2 at 16:34
8
\$\begingroup\$

Class methods calling each other is perfectly acceptable in general, but this should be done toward the end of making the code easier to follow by encapsulating complexity. Two things in your example potentially work against that goal:

  1. All of the methods and attributes are public. (In Python you denote a "private" member by starting its name with _.)
  2. Data is passed between the methods via instance attributes.

This means that someone reviewing this code for possible bugs needs to assume that a caller might call any of these methods while the object is in any arbitrary state -- that's a lot of combinations to consider!

If the only intended usage of this class is the one you've given (create a single instance, call create_repository exactly once), then there isn't much benefit to making it a class with state (since that state is never meant to be reused beyond that one function call). The other typical way of grouping related functions is to simply define them locally within a larger function:

def create_repository(project_name: str, repo_name: str) -> None:
    """Creates a new repo under the specified project"""

    with open("./repo_request_body.json", "r") as file:
        data = json.load(file)

    def get_project_id() -> str:
        """Returns the Azure DevOps project ID for a project name"""
        _request_object = make_request(
            "_apis/projects/",
            project_name,
            "?api-version=5.1-preview.1",
            request_method = "get"
        )
        return _request_object.get("id")

    def update_project_id() -> None:
        """Updates JSON dictionary with new ID"""
        new_id = get_project_id()
        data["project"]["id"] = new_id

    def update_repo_name() -> None:
        """Updates JSON dictionary with new repo name"""
        data["name"] = repo_name

    json_content_type = {
        'Content-Type': 'application/json',
        'Authorization': 'Basic XXXXXXXXXXXXXXXX'
        }

    update_project_id()
    update_repo_name()
    make_request(
        project_name,
        "/_apis/git/repositories",
        "?api-version=5.1-preview.1",
        request_method = "post",
        data = json.dumps(data),
        headers = json_content_type
    )

create_repository("project_name", "repo_name")

But given that each of these functions is only called once, is there any value to actually giving them individual names and making the reader jump around between them? I'd probably just write this as one function where everything is written in exactly the order it happens in:

def create_repository(project_name: str, repo_name: str) -> None:
    """Creates a new repo under the specified project"""
    # Load the repo request data from disk.
    with open("./repo_request_body.json", "r") as file:
        data = json.load(file)

    # Get the Azure DevOps project ID for a project name
    _request_object = make_request(
        "_apis/projects/",
        project_name,
        "?api-version=5.1-preview.1",
        request_method="get"
    )
    new_id = _request_object.get("id")

    # Update JSON dictionary with new ID
    data["project"]["id"] = new_id

    # Update JSON dictionary with new repo name
    data["name"] = repo_name

    # Make the server request.
    json_content_type = {
        'Content-Type': 'application/json',
        'Authorization': 'Basic XXXXXXXXXXXXXXXX'
    }
    make_request(
        project_name,
        "/_apis/git/repositories",
        "?api-version=5.1-preview.1",
        request_method="post",
        data=json.dumps(data),
        headers=json_content_type
    )

create_repository("project_name", "repo_name")

IMO this is a lot easier to follow, in that dependencies between the different blocks of code are very obvious and I can read it from top to bottom and easily keep track of what's happening when.

Taking it one step further, I'd eliminate the named variables that only get used once, which shortens the new_id part of the code (again, less for the reader to keep track of if they can see at a glance that function calls chain together to produce a single result, rather than having to try to figure out if a given value is going to be used again later):

def create_repository(project_name: str, repo_name: str) -> None:
    """Creates a new repo under the specified project"""
    # Load the repo request data from disk.
    with open("./repo_request_body.json", "r") as file:
        data = json.load(file)

    # Update JSON dictionary with new ID from server API
    data["project"]["id"] = make_request(
        "_apis/projects/",
        project_name,
        "?api-version=5.1-preview.1",
        request_method="get"
    ).get("id")

    # Update JSON dictionary with new repo name
    data["name"] = repo_name

    # Make the server request to create the repo.
    make_request(
        project_name,
        "/_apis/git/repositories",
        "?api-version=5.1-preview.1",
        request_method="post",
        data=json.dumps(data),
        headers={
            'Content-Type': 'application/json',
            'Authorization': 'Basic XXXXXXXXXXXXXXXX',
        }
    )
| improve this answer | | | | |
\$\endgroup\$
4
\$\begingroup\$

I think in this case, where these are the only methods of the class, you don't need the class at all. Just make it standalone functions:

import json

JSON_CONTENT_TYPE = {
    'Content-Type': 'application/json',
    'Authorization': 'Basic XXXXXXXXXXXXXXXX'
    }


def make_request(...):
    ...


def get_project_id(project_name):
    """Returns the Azure DevOps project ID for a project name"""
    url = f"_apis/projects/{project_name}?api-version=5.1-preview.1"
    return make_request(url, request_method="get").get("id")


def create_repository(project_name, repo_name):
    """Creates a new Azure DevOps repo under the specified project"""
    with open("./repo_request_body.json") as file:
        data = json.load(file)
    data["project"]["id"] = get_project_id(project_name)
    data["name"] = repo_name

    url = f"{project_name}/_apis/git/repositories?api-version=5.1-preview.1"
    make_request(url, request_method="post",
                 data=json.dumps(data), headers=JSON_CONTENT_TYPE)

This uses Python's official style-guide, PEP8, which recommends no spaces around = when using it for keyword arguments, UPPER_CASE for global constants and the relatively new f-string to make formatting strings easier. Since you did not supply the make_request function I am not sure if that is how it is used, though.

You might want to make the API-version also an argument of the function (maybe with a default argument).

| improve this answer | | | | |
\$\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.