-
Notifications
You must be signed in to change notification settings - Fork 0
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’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Review #1
Comments
Hi Evgeny,
Thank you for this detailed and informative feedback.
I will surely get to better understand the rules you pointed out.
Anyways, I had a lot of fun doing this project and will try to become a
JetBrains intern in the future.
Thanks,
Ernest Mołczan
W dniu wt., 7.05.2024 o 20:36 Evgeniy Stepanov ***@***.***>
napisał(a):
… Hello Ernest,
thank you for participating JetBrains Intership program and for choosing
my internship project! My name is Evgeny, I am Team Lead in Rider Platform.
Now I am going to provide you a comprehensive code review.
First of all, you written excellent readme.md file with detailed
explanation of your vision and various feature. I liked it. Do the same in
the future.
About UX. I am not sure I like your desing because it is sometimes
difficult to read black code symbols on dark-gray background. Also I
constantly feel application is unfocuced because of that background color.
The code is quite simple and easy-to-read. It is fine. Also you made
avalonia integration well, left a good amount of comments in business logic
and tried to split view and view model. However, the lack of industrial
programming experience is very noticeable. Below there are several examples
which I suggest you to improve.
1. Do not put business logic into code behind. For example in
OnTreeCaretEvent you mixed up some AST/Text operations and UI updates.
Usually this approach leads to the spaghetti code over the years.
https://github.com/molczane/CodeVisualizer/blob/4275e6ec53e617d64f1766570376c20d20f93239/RoslynAvaloniaTask/Views/MainWindow.axaml.cs#L113
2. There are code duplicates. Cleanup such things before final
commit/merge/review. Code base should be clean and straightforward.
https://github.com/molczane/CodeVisualizer/blob/4275e6ec53e617d64f1766570376c20d20f93239/RoslynAvaloniaTask/Views/MainWindow.axaml.cs#L210
https://github.com/molczane/CodeVisualizer/blob/4275e6ec53e617d64f1766570376c20d20f93239/RoslynAvaloniaTask/ViewModels/MainWindowViewModel.cs#L61
3. Meaningless or unused code. Try to avoid it.
https://github.com/molczane/CodeVisualizer/blob/4275e6ec53e617d64f1766570376c20d20f93239/RoslynAvaloniaTask/ViewModels/MainWindowViewModel.cs#L107
https://github.com/molczane/CodeVisualizer/blob/4275e6ec53e617d64f1766570376c20d20f93239/RoslynAvaloniaTask/Views/MainWindow.axaml.cs#L150
4.
Naming. private StackPanel stackPanel; but one line above private
SyntaxTreeCaretEvent _treeCaretEvent;
5.
StringBuilder is your friend when you work with strings.
https://github.com/molczane/CodeVisualizer/blob/4275e6ec53e617d64f1766570376c20d20f93239/RoslynAvaloniaTask/Views/MainWindow.axaml.cs#L243
The general suggestion is to pay more attention to small details and code
tidiness. While this may not seem important for a one-day home project, it
becomes critical in real production. It's better to demonstrate the ability
to write clean code and have knowledge not only of .NET or Avalonia
frameworks, but also of MVC/MVVM and SOLID. This may sound a bit
'old-school', but I have to repeat, it becomes crucial in production.
—
Reply to this email directly, view it on GitHub
<#1>, or unsubscribe
<https://github.com/notifications/unsubscribe-auth/A6S25W75DAISGSN5NLQNY3DZBENMBAVCNFSM6AAAAABHLRB7N6VHI2DSMVQWIX3LMV43ASLTON2WKOZSGI4DGOJYG42DONY>
.
You are receiving this because you are subscribed to this thread.Message
ID: ***@***.***>
|
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Hello Ernest,
thank you for participating JetBrains Intership program and for choosing my internship project! My name is Evgeny, I am Team Lead in Rider Platform. Now I am going to provide you a comprehensive code review.
First of all, you written excellent
readme.md
file with detailed explanation of your vision and various feature. I liked it. Do the same in the future.About UX. I am not sure I like your desing because it is sometimes difficult to read black code symbols on dark-gray background. Also I constantly feel application is unfocuced because of that background color.
The code is quite simple and easy-to-read. It is fine. Also you made avalonia integration well, left a good amount of comments in business logic and tried to split
view
andview model
. However, the lack of industrial programming experience is very noticeable. Below there are several examples which I suggest you to improve.OnTreeCaretEvent
you mixed up some AST/Text operations and UI updates. Usually this approach leads to the spaghetti code over the years.CodeVisualizer/RoslynAvaloniaTask/Views/MainWindow.axaml.cs
Line 113 in 4275e6e
CodeVisualizer/RoslynAvaloniaTask/Views/MainWindow.axaml.cs
Line 210 in 4275e6e
CodeVisualizer/RoslynAvaloniaTask/ViewModels/MainWindowViewModel.cs
Line 61 in 4275e6e
CodeVisualizer/RoslynAvaloniaTask/ViewModels/MainWindowViewModel.cs
Line 107 in 4275e6e
CodeVisualizer/RoslynAvaloniaTask/Views/MainWindow.axaml.cs
Line 150 in 4275e6e
Naming.
private StackPanel stackPanel;
but one line aboveprivate SyntaxTreeCaretEvent _treeCaretEvent;
StringBuilder
is your friend when you work with strings.CodeVisualizer/RoslynAvaloniaTask/Views/MainWindow.axaml.cs
Line 243 in 4275e6e
The general suggestion is to pay more attention to small details and code tidiness. While this may not seem important for a one-day home project, it becomes critical in real production. It's better to demonstrate the ability to write clean code and have knowledge not only of .NET or Avalonia frameworks, but also of MVC/MVVM and SOLID. This may sound a bit 'old-school', but I have to repeat, it becomes crucial in production.
The text was updated successfully, but these errors were encountered: