Code Reviews Example 1 Uncle Bob Args - herougo/SoftwareEngineerKnowledgeRepository GitHub Wiki
Original link: https://github.com/unclebob/javaargs
Code (Adapted to Python by me)
from enum import Enum
class ArgumentMarshaler:
def set_arg(current_argument):
raise NotImplementedError()
@classmethod
def get_value(cls, marshaller):
if marshaller is None:
return cls._value
else:
return marshaller._value
class ErrorCode(Enum):
OK = 0
INVALID_ARGUMENT_FORMAT = 1
UNEXPECTED_ARGUMENT = 2
INVALID_ARGUMENT_NAME = 3
MISSING_STRING = 4
MISSING_INTEGER = 5
INVALID_INTEGER = 6
class BooleanArgumentMarshaler(ArgumentMarshaler):
_value = False
def set_arg(self, string_iterator):
self._value = True
class IntegerArgumentMarshaler(ArgumentMarshaler):
_value = 0
def set_arg(self, string_iterator):
try:
self._value = int(next(string_iterator))
except ValueError as ex:
raise ArgsException(ErrorCode.INVALID_INTEGER.value)
except IndexError as ex:
raise ArgsException(ErrorCode.MISSING_INTEGER.value)
class StringArgumentMarshaler(ArgumentMarshaler):
_value = ''
def set_arg(self, string_iterator):
try:
self._value = next(string_iterator)
except IndexError as ex:
raise ArgsException(ErrorCode.MISSING_STRING.value)
def is_letter(c):
if len(c) != 1:
return False
ord_c = ord(c)
return ((ord('A') <= ord_c and ord_c <= ord('Z')) or
(ord('a') <= ord_c and ord_c <= ord('z')))
class ArgsException(Exception):
def __init__(self, error_code, error_argument_id=None, error_parameter=None):
self._error_code = error_code
self._error_argument_id = error_argument_id
self._error_parameter = error_parameter
super().__init__(self.error_message)
def set_error_argument_id(self, error_argument_id):
self._error_argument_id = error_argument_id
@property
def error_message(self):
if self._error_code == ErrorCode.OK.value:
return "TILT: Should not get here."
elif self._error_code == ErrorCode.UNEXPECTED_ARGUMENT.value:
return f"Argument -{self._error_argument_id} unexpected."
elif self._error_code == ErrorCode.MISSING_STRING.value:
return f"Could not find string parameter for -{self._error_argument_id}."
elif self._error_code == ErrorCode.INVALID_INTEGER.value:
return (f"Argument -{self._error_argument_id} expects an integer but "
f"was '{self._error_parameter}'.")
elif self._error_code == ErrorCode.MISSING_INTEGER.value:
return f"Could not find integer parameter for -{self._error_argument_id}."
elif self._error_code == ErrorCode.INVALID_ARGUMENT_NAME.value:
return f"'{self._error_argument_id}' is not a valid argument name."
elif self._error_code == ErrorCode.INVALID_ARGUMENT_FORMAT.value:
return f"'{self._error_parameter}' is not a valid argument format."
return ""
class Args:
def __init__(self, schema, args):
self._marshalers = dict() # maps character to ArgumentMarshaler
self._args_found = set()
self._args_iter = None
self._parse_schema(schema)
self._parse_argument_strings(args)
def _parse_schema(self, schema):
for element in schema.split(','):
if len(element) > 0:
self._parse_schema_element(element.strip())
def _parse_schema_element(self, element):
element_id, element_tail = element[0], element[1:]
self._validate_schema_element(element_id)
if len(element_tail) == 0:
self._marshalers[element_id] = BooleanArgumentMarshaler()
elif element_tail == '*':
self._marshalers[element_id] = StringArgumentMarshaler()
elif element_tail == '#':
self._marshalers[element_id] = IntegerArgumentMarshaler()
else:
raise ArgsException(ErrorCode.INVALID_ARGUMENT_FORMAT.value, element_id,
element_tail)
def _validate_schema_element(self, element_id):
if not is_letter(element_id):
raise ArgsException(ErrorCode.INVALID_ARGUMENT_NAME.value, element_id, None)
def _parse_argument_strings(self, args):
# (rewritten to not work with -dfg)
self._args_iter = iter(args)
current = next(self._args_iter, None)
while current is not None:
if current.startswith('-'):
self._parse_argument_character(current[1])
else:
assert False
current = next(self._args_iter, None)
def _parse_argument_character(self, arg_char):
m = self._marshalers.get(arg_char, None)
if m is None:
raise ArgsException(ErrorCode.UNEXPECTED_ARGUMENT.value, arg_char, None)
else:
self._args_found.add(arg_char)
try:
m.set_arg(self._args_iter)
except ArgsException as ex:
ex.set_error_argument_id(arg_char)
raise ex
def get_boolean(self, char):
m = self._marshalers.get(char, None)
return BooleanArgumentMarshaler.get_value(m)
def get_int(self, char):
m = self._marshalers.get(char, None)
return IntegerArgumentMarshaler.get_value(m)
def get_string(self, char):
m = self._marshalers.get(char, None)
return StringArgumentMarshaler.get_value(m)
def main(args):
try:
arg = Args('l,p#,d*', args)
logging = arg.get_boolean('l')
port = arg.get_int('p')
directory = arg.get_string('d')
execute_application(logging, port, directory)
except Exception as ex:
print(f'Argument error: {str(ex)}')
def execute_application(logging, port, directory):
print(f"logging is {logging}, port:{port}, directory:{directory}")
main('-l -p 8080 -d /home/yo/Documents'.split(' '))
Review
Criticism
- what is a marshaler in this context?
- do you really need a marshaler object for each arg?
- alternative: just use functions and route to the appropriate function
- in python, you can have a dictionary mapping argument_type to parse_argument function (since switch statements don't exist)
- alternative: just use functions and route to the appropriate function
- minor
- _validate_schema_element does not need to be its own function?
Praise
- I like separating the error_message into the arguments class
- looks good (nice balance of creating new methods such as _parse_schema)
Someone's Rewriting of the Original Java Code
This video claims that OOP is bad and gives 4 examples.
https://youtu.be/IRTfhkiAqPw?t=1376
However, a better title should be that "bad OOP is bad". Furthermore, his rewriting of Uncle Bob's code is IMO worse.
Criticism
- Code is shorter, but at a cost of readability (case and point the Args constructor)