There's more to quality of implementation that a crash/not crash scenario.
Many metrics can be used, ranging from code legibility through execution speed, memory efficiency, lines of code, even API simplicity, predictability and ease of use and there's also documentation. Managers like to keep a track of time taken to deliver.
Quality implementations would score as high as possible on all those metrics and more. It's not always possible to reconcile all of these.
Code reviews tend to work well to improve code, but it is imperative that the process is devoid of emotion and is purely focused on technical details. Good code is often a result of revising and discarding many alternatives - it's not uncommon to spend most of the effort on code that will never make it.
As an example of what code review might bring to the table, here is a recent commit from OpenViX:
Code: Select all
+ def first(self):
+ if self.instance is not None:
+ self.instance.moveSelection(self.instance.moveTop)
+
+ def last(self):
+ if self.instance is not None:
+ self.instance.moveSelection(self.instance.moveEnd)
+
def pageUp(self):
if self.instance is not None:
self.instance.moveSelection(self.instance.pageUp)
def pageDown(self):
if self.instance is not None:
self.instance.moveSelection(self.instance.pageDown)
If I was involved in the code review for that, I would be asking why the proxy API uses first/last nomenclature for the functions that operate on the underlying eListbox. I would be suggesting that first/last should follow the underlying eListbox naming scheme and the functions should be called moveTop/moveEnd or similar. The reason being a predictable and uniform API - you don't want to work too hard on having to remember the correct synonym for the same actions depending on the context. I would expect either a good technical argument for keeping the code or a code change that implements the suggestion.
So, going back to your question... Whether I welcome your code changes will entirely depend on the code itself. OpenViX have a tendency to merge first and ask questions (or revert) later, so I suspect that what I think won't matter anyway. It'll just come down the pipeline. If you want input on the code, you can use Github or Bitbucket to push code on a topic branch and then invite people to take a look and comment. No guarantees that I will have time to be thorough, but I'll try to take a peek.
Please do keep in mind that criticism of the code is all about improving the code. It is in no way aimed at you and should not be taken as criticism of you as a person.
