Skip to content

Code review - #126

Open
aurelPierre wants to merge 4 commits into
devfrom
code_review
Open

Code review#126
aurelPierre wants to merge 4 commits into
devfrom
code_review

Conversation

@aurelPierre

Copy link
Copy Markdown
Collaborator

/!\ DO NOT MERGE THIS PULL REQUEST /!\

Welcome to this Code Review !

I made a lot of comments in the entire code base (including cmake) however, I didn't look at the algorithm you wrote. Indeed, I only looked at the code globally, it means c++ syntax, semantics, formats, etc...

Some errors happen more than others like:

  • Rule of three
  • Nomenclature
  • Empty constructor & destructor

Feel free to comment the comment via github if you want more information about them.

After looking at your code, I think you are not rigorous enough. Your nomenclature is all over the place, there are a lot of typos and formatting errors. It's nothing dramatic, but it's tiresome in the long run.
There is another point that I want to address, it's about your methodology. When you are about to implement a feature, there a few questions you must ask yourself:

  • What is this feature about ? (obviously)
  • Why is it needed ?
  • Who will use it ?
  • How could it fit inside the code ?
  • Do I have everything I need to implement the feature ?
  • What are the edge case ?

When I see some of your code, I don't know if you asked yourselves these questions.

Well, the code itself is okay overall so don't fret yourself about this too much. They are only remarks to help you become better programmers !

Good luck and continue working hard !

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants