Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

Code review and general feedback #24

Open
petrspelos opened this issue Nov 20, 2019 · 3 comments
Open

Code review and general feedback #24

petrspelos opened this issue Nov 20, 2019 · 3 comments
Assignees
Labels
documentation Improvements or additions to documentation enhancement New feature or request
Milestone

Comments

@petrspelos
Copy link

Overall

We went through this repository with @DraxCodes. Overall, we like the idea of it. The Windows 10 style notifications are a nice touch. 馃檪 馃憤

@DraxCodes had a bit of an issue setting it up, we had to comment out the Timezone action, because it just wouldn't work with it. He can perhaps give you more detail about that specific issue.

Easy to fix

Let's start with the things that can be really easily fixed. One of the things most repositories have is a set of documents that help other people contribute to your project. And since you seem to be quite passionate about letting other people contribute, I think you should add these documents. 馃槉

I recommend you checkout the Open Source guide for what you can add to your project.

Here's a couple of things to focus on:

  • Extend your README.md file with information about what the application really does, give examples for trying it out, some information about how to set it up. etc... Refer to the Open Source guide for more details on how to make a great readme. 馃槈
  • Add a LICENSE file. Depending on how you see Open Source or Free Software, you might want to pick a license such as GPL or MIT. See the guide for picking a license. 馃檪
  • Add a CONTRIBUTING.md file describing the code standards you want others to follow in your project. This usually mentions git branches, code style, and similar standards.
  • Adopt CODE_OF_CONDUCT.md refer to the guide for more information about the code of conduct document.

Project structure

Maybe having both your .sln (solution file) and your .csproj (project file) in the root of the repository isn't the most standard solution. While it's fine, it is more common to see a structure where your project (along with your .csproj is in a directory of it's own).
For example:

YourRepo
    - Solution.sln
    - Project (folder)
        - Project.csproj
        - Other project files

Naming convention

It's also quite common to start your .cs files with a capital letter.
So renaming question.cs to Question.cs. Easy fix. 馃槉

Code Standards

It might be worth adopting one of the common C# Code standards. Take a look at Microsoft's Coding Conventions page for inspiration.

One thing @DraxCodes mentioned is instead of having your questions be hardcoded in the questions.cs file, it might be worth looking into a system where you could store them in a JSON file. I quite like that idea.

Performance

You did request a special focus on performance, but running your project the resource usage was next to none. That's actually great news, I'd say.

It is commonly said that premature optimization "is the root of all evil". 馃槄 So I wouldn't worry about performance at this stage.

On that note, however, there are a couple of things that could be improved.
In your Bootstrap.cs file, you iterate through all of your services and actually executing them to figure out which is supposed to return a result. Moreover, you stop at the first one that matches.

It might be worth adding a sort of bool Matches(string message) method to your IAction. This method could just very quickly check the message for if it's able to be parsed with this action. You'd probably want to do that with something like regular expressions.

With that, you could use System.Linq to change your foreach into:

var service = serviceProvider.GetServices<IAction>().FirstOrDefault(s => s.Matches(clipboardText);

Which is quite simpler to read.

Final suggestions

It might be worth looking into some resources for inspiration on improvements. For example the free PDF book The Art of Readable code is really easy to read, quite fun, and contains important concepts. 馃槉 I recommend that one.

Overall, you've got a very nice project. The things I mentioned are just suggestions. You may decide to not implement some of them, or even implement some that I didn't mention. That's perfectly fine as long as you keep learning and having fun. 鉂わ笍 馃憤

@RyanThiele
Copy link
Contributor

I wanted to point out that in the bootstrap, it will iterate through the services in the order they were added in ConfigureDependancies() method.

I agree with a descriptive readme and license.

@StijnWoerkom
Copy link
Collaborator

@petrspelos I agree with you. I know this guy and the problem is that he doesn't have the experience for this. When I see him again, I will talk with him. Thanks for your feedback!

@StijnWoerkom StijnWoerkom added documentation Improvements or additions to documentation enhancement New feature or request labels Jan 28, 2020
@StijnWoerkom
Copy link
Collaborator

README, CONTRIBUTING and an MIT license are added. Now working on Code of Conduct.

@StijnWoerkom StijnWoerkom added this to the V_1.0.0 milestone Jan 28, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
documentation Improvements or additions to documentation enhancement New feature or request
Projects
None yet
Development

No branches or pull requests

4 participants