3 ms·

I'm curious why the author chose to put every command in its own file and then copy the big if tree with string compares in every file. Translating commands fr

I'm curious why the author chose to put every command in its own file and then copy the big if tree with string compares in every file. Translating commands from a generic command interface to specific commands is a great case for polymorphism.

akx 8y ago
My guess is they didn't know better. That isn't the only amateurism in the code.
- deleted 8y ago
  [deleted]
xaedes 8y ago
I don't really see what this program would gain from an "enterprise fizzbuzz" implementation. It is simple, it works, everyone can understand the code and update it. That is actually very valuable, especially for a project like this which tends to need maintenance to keep alive (whenever some wrapped command changes).
- vbsteven 8y ago
  Adding polymorphism for easier maintenance does not mean "enterprise fizzbuzz". I'm talking about one 1 abstract class to define the interface and 1 implementation class per package manager. That's N+1 classes (N = # of package managers). It means all logic for a specific package manager is gathered in one place instead of spread out over many files and the compiler can prevent us making stupid mistakes like forgetting to add a subcommand or mistyping the packagename in the big if/if/if statement. In the current implementation we have (X = numberOfPMs*numberOfCommands) string compares which are not checked by the compiler. That means X potential typo's that each need a unit test just to verify that the implementation is hooked up correctly. I would say that a class based implementation like this is simpler and prevents more errors during maintenance than the current version. Don't put down OOP and classes just because hating on GoF patterns is all the rage in 2018, they exist for a reason. This is a clear case for applying them.
  - xaedes 8y ago
    I think the best point for class implementation you mention is to have all the related stuff in one place. That convinces me most =)
franga2000 8y ago
That bothered me at first, but thinking about it more, I actually don't mind it in such a small project for one reason: I'd expected that at least 95% of first-year programming students could jump in and add support for a new package manager without any trouble. By contrast, none of the students in my 3rd-year Java class could do the same in a version implemented with polymorphism. This is the perfect project to get new programmers excited about contributing to open source. The one change I would suggest is putting the package manager names as constants all in one place. That way, you can't make a typo in one of the checks in the if statements. That, and maybe replacing the if-else chain with a switch statement. Might help remove some duplication

akx 8y ago

My guess is they didn't know better. That isn't the only amateurism in the code.

deleted 8y ago
[deleted]

xaedes 8y ago

I don't really see what this program would gain from an "enterprise fizzbuzz" implementation. It is simple, it works, everyone can understand the code and update it. That is actually very valuable, especially for a project like this which tends to need maintenance to keep alive (whenever some wrapped command changes).

vbsteven 8y ago
Adding polymorphism for easier maintenance does not mean "enterprise fizzbuzz". I'm talking about one 1 abstract class to define the interface and 1 implementation class per package manager. That's N+1 classes (N = # of package managers). It means all logic for a specific package manager is gathered in one place instead of spread out over many files and the compiler can prevent us making stupid mistakes like forgetting to add a subcommand or mistyping the packagename in the big if/if/if statement. In the current implementation we have (X = numberOfPMs*numberOfCommands) string compares which are not checked by the compiler. That means X potential typo's that each need a unit test just to verify that the implementation is hooked up correctly. I would say that a class based implementation like this is simpler and prevents more errors during maintenance than the current version. Don't put down OOP and classes just because hating on GoF patterns is all the rage in 2018, they exist for a reason. This is a clear case for applying them.
- xaedes 8y ago
  I think the best point for class implementation you mention is to have all the related stuff in one place. That convinces me most =)

franga2000 8y ago

That bothered me at first, but thinking about it more, I actually don't mind it in such a small project for one reason: I'd expected that at least 95% of first-year programming students could jump in and add support for a new package manager without any trouble. By contrast, none of the students in my 3rd-year Java class could do the same in a version implemented with polymorphism. This is the perfect project to get new programmers excited about contributing to open source. The one change I would suggest is putting the package manager names as constants all in one place. That way, you can't make a typo in one of the checks in the if statements. That, and maybe replacing the if-else chain with a switch statement. Might help remove some duplication