Pysyft: AST Refactor

Created on 2 Aug 2020 · 4Comments · Source: OpenMined/PySyft

Description

The AdditiveSharingTensor class can be refactored:
Some (but not all) the things that could be improved:

methods that are less complex;
the methods that are not specific for an instance could be made staticmethods (decorator)
use a pythonic naming convention: leading _ for what are private methods
init_shares can be renamed to share_secret

Testing

All the tests work as expected

Status Type

Source

gmuraru

Most helpful comment

Hi @marload :) thank you for taking this issue, we made this issue to enhance the readability of this class, this should be your primary goal to make it more readable, I'll share some tips with you :)

the function is readable if you're able to know what it's doing without having to look in the detailed implementation. you should read it like you read a newspaper.
to read a function replace "def" with "to"
for example, this function reads like "to share a secret, you get the number of shares then you arrange the workers then you generate the shares then you distribute the shares then you save a map to them as the tensor child"
you should pick a good (expressive) name for every function, parameter or variable, use verbs for functions and nouns for vars
a function must have a single responsibility that is expressed by its name, only one, that's so important. many of the functions in this class has more than one and they need to be broken down into smaller functions.
functions should be small, like 5 lines long, the shorter the better
abstract concepts should come first in the file then more details are reviled down the lines
utility functions should be near the functions that use them
avoid encoding, write the whole thing instead, (e.x. library not lib nor librry)
use naming convention from CS (hash map, list ..etc) or business domain or scientific paper
this is more of an art than engineering, I'll be seeing your art :) good luck

abogaziah on 3 Aug 2020

👍2

All 4 comments

May I take this issue?

marload on 3 Aug 2020

Sure! It might involve more refactoring than it is written in the description.

gmuraru on 3 Aug 2020

👍1

the function is readable if you're able to know what it's doing without having to look in the detailed implementation. you should read it like you read a newspaper.
to read a function replace "def" with "to"
for example, this function reads like "to share a secret, you get the number of shares then you arrange the workers then you generate the shares then you distribute the shares then you save a map to them as the tensor child"
you should pick a good (expressive) name for every function, parameter or variable, use verbs for functions and nouns for vars
a function must have a single responsibility that is expressed by its name, only one, that's so important. many of the functions in this class has more than one and they need to be broken down into smaller functions.
functions should be small, like 5 lines long, the shorter the better
abstract concepts should come first in the file then more details are reviled down the lines
utility functions should be near the functions that use them
avoid encoding, write the whole thing instead, (e.x. library not lib nor librry)
use naming convention from CS (hash map, list ..etc) or business domain or scientific paper
this is more of an art than engineering, I'll be seeing your art :) good luck

abogaziah on 3 Aug 2020

👍2

This issue has been marked stale because it has been open 30 days with no activity. Leave a comment or remove the stale label to unmark it. Otherwise, this will be closed in 7 days.