filiphajek
commented
1 year ago I know that we are not required to fix these issues. But I feel that I should comment on some of your notes and provide a better solution.
I agree that there should not be a limit to the length of arguments or how many parameters the user passes to the application. To be honest I can not really tell why I did it like this, the only reason I remember is that I wanted to stop the user from doing something weird (passing too many arguments that are obviously not necessary). But obviously, this is a weak argument .. our implementation (ArgumentParser) should handle long parameter list just fine without these limitations like MaxArgumentLength. I can add some tests to make sure that we can handle these long inputs.
About Early Return technique... I again agree that there are some functions that would benefit from this technique (3 nested if statements .. not the best solution). We use this technique in some places (e.g. checking cli parameters) so we will try to improve/refactor other parts of our implementation too.
These are just general observations I made when reading your code. You are not required to fix any of these but may give you some suggestions on what to improve.
Why is the limitation of argument lengths necessary? POSIX file systems support paths far longer than what
MaxArgumentLengthis set to.Similarly, explicitly limiting the number of arguments is a weird thing to do. Why exactly 20? Why is 19 okay? What is the reason behind these numbers? Can you prove (preferably formally) that no correct argument list has at least 20 elements?
Ultimately you should conclude that it is not the length of the argument list that matters here.
Using regex to parse options is overkill.
Skimming through the code, many functions would benefit from employing the Early Return technique.