Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Possible minor code improvements #63

Open
Solomon1732 opened this issue Feb 18, 2021 · 5 comments
Open

Possible minor code improvements #63

Solomon1732 opened this issue Feb 18, 2021 · 5 comments
Labels
enhancement New feature or request

Comments

@Solomon1732
Copy link
Contributor

Solomon1732 commented Feb 18, 2021

In mind I have a small number of possible (minor) improvements:

Replace % string operators with f-strings

Using the % operator is somewhat brittle AFAIK, and can arguably be less readable.
Example:
https://github.com/Tribler/anydex-core/blob/master/anydex/core/tickentry.py#L144

Replace list comprehensions in str.join with generator expressions

Building a list for a str.join statement is an operation which can be skipped. Instead it can be replaced with a generator expression.
Example:
https://github.com/Tribler/anydex-core/blob/master/anydex/core/match_queue.py#L13

# This
return ' '.join([str(i) for i in self.queue])
# Can be replaced with this
return ' '.join(str(i) for i in self.queue)

Remove object as parent class

Since the default parent class of new classes in Py3 is object by default, such a declaration became redundant. Such declarations can be deleted with no adverse effects.
Example:
https://github.com/Tribler/anydex-core/blob/master/anydex/core/settings.py

Are points 1 and 3 artifacts from transitioning from Py2 to Py3?

I know these are all minor points. I don't mind creating one or more PRs for what I suggested, and in fact I'll be glad to make them since I am aware that such issues are of a low priority.

Edit:

Replace two argument form calls to super() with the zero argument form when appropriate

https://docs.python.org/3/library/functions.html#super
https://stackoverflow.com/a/576183
Code such as

class Cls:
    def __init__(*args, **kwargs):
        super(Cls, self).__init__():

can be replaced with

class Cls:
    def __init__(*args, **kwargs):
        super().__init__():

The two are equivalent.
Example:
https://github.com/Tribler/anydex-core/blob/master/anydex/core/message.py#L13

@Solomon1732 Solomon1732 changed the title Possible three minor code improvements Possible minor code improvements Feb 18, 2021
@Solomon1732
Copy link
Contributor Author

Solomon1732 commented Feb 18, 2021

Looking through the code I ran into MatchPriorityQueue. Internally it uses tuples as entries. It looks like the aspect of code readability it could benefit if typing.NamedTuples are used. As far as I am aware using them isn't significantly more expensive (performance-wise) than using plain tuples.
Example:

# This
def contains_order(self, order_id):
    for _, _, other_order_id, _ in self.queue:
        if other_order_id == order_id:
            return True
    return False

# Might look like this
def contains_order(self, order_id):
    for other_order in self.queue:
        if other_order.id == order_id:
            return True
    return False

@Solomon1732
Copy link
Contributor Author

Solomon1732 commented Feb 18, 2021

https://github.com/Tribler/anydex-core/blob/master/anydex/core/database.py#L128
The code in the link could be replaced with something like the following:

def get_order(self, order_id):
    """
    Return an order with a specific id.
    """
    db_result = next(
        self.execute(
            "SELECT * FROM orders WHERE trader_id = ? AND order_number = ?",
            database_blob(bytes(order_id.trader_id), str(order_id.order_number))
        ),
        # This is an optional default value which can be returned
        None,
    )
    return None if db_result is None else Order.from_database(db_result, self.get_reserved_ticks(order_id))

This prevents the need to set up a try-except for StopIteration

Edit:

Replace ValueError with TypeError when checking for types

When checking for types and the wrong type is found it might be more appropriate to raise a TypeError instead of a ValueError.
Example:
https://github.com/Tribler/anydex-core/blob/master/anydex/core/timestamp.py#L16

@devos50
Copy link
Contributor

devos50 commented Feb 19, 2021

Thanks for your suggestions!

Recently, the ideas implemented in this repository (low-risk, universal asset trading) have been accepted for publication. Also, the matchmaking logic has recently been published. I think further improvements can be made to the exchange mechanism but we consider the fundamental work as finished. As such, the future plans of this repository are not clear yet.

Even though we highly value any contribution, your time might be better spent working on one of our other projects, for example, Tribler, our mobile superapp or our accountability toolbox. This would also depend on how much time you would like to invest into one of our projects, and your interests/expertise 👍 . Do you have any preferences/ideas?

@Solomon1732
Copy link
Contributor Author

Gotcha. Happy to help!

I have just above beginner skills in python, and I can read C code, but that's it. Do you have any suggestions?

@devos50
Copy link
Contributor

devos50 commented Feb 19, 2021

It depends also on what you like to work on. You can, for example:

  • Implement the suggestions on this repository. I will review your changes and merge them. Since this code is not used in any other project, the risks of breaking something important is absolutely minimal.
  • Explore the code of our accountability toolbox. Note that this code is in an early stage and might be a bit harder to understand.

@devos50 devos50 added the enhancement New feature or request label Feb 23, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants