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’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Code Quality: Code cleanup and formatting #11156

Merged
merged 16 commits into from
Feb 12, 2023

Conversation

0x5bfa
Copy link
Member

@0x5bfa 0x5bfa commented Feb 4, 2023

Resolved / Related Issues

None

Change Overview

  • Remove unnecessary line breaks
  • Add line breaks for improving readability
  • Use Lambda expression
  • Use pattern matching
  • Make C# files to end with '\r\n\0' (a line break and EOF)
  • Simplify type name
  • Simplify expression
  • Capitalize the first word of each sentence in comment block.
  • Use 'TODO' and 'NOTE', instead of 'todo', 'note', or anything else
  • Use '//' for commenting our, instead of '/* */'
  • Use if, else embedded statement when it is one line.
  • Use switch expression

Unchanged files

  • Dialogs/*
  • Views/*
  • Filesystem/*
  • Helpers/*

Validation
How did you test these changes?

  • Built and ran the app
  • Tested the changes for accessibility

Screenshots (optional)

None

@0x5bfa 0x5bfa changed the title CodeQuality: Improve the quality of almost all codes CodeQuality: Improve the quality of almost of all codes Feb 4, 2023
@hecksmosis
Copy link
Contributor

Oh god, this is going to be fun to review

Copy link
Contributor

@ferrariofilippo ferrariofilippo left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good, except for some cases

src/Files.App/BaseLayout.cs Outdated Show resolved Hide resolved
src/Files.App/BaseLayout.cs Outdated Show resolved Hide resolved
src/Files.App/CommandLine/CommandLineParser.cs Outdated Show resolved Hide resolved
src/Files.App/ViewModels/StatusCenterViewModel.cs Outdated Show resolved Hide resolved
src/Files.App/ViewModels/StatusCenterViewModel.cs Outdated Show resolved Hide resolved
src/Files.App/ViewModels/ToolbarViewModel.cs Outdated Show resolved Hide resolved
@0x5bfa
Copy link
Member Author

0x5bfa commented Feb 8, 2023

@ferrariofilippo Done! Thank you for reviewing all of this, anyway.

@0x5bfa 0x5bfa changed the title CodeQuality: Improve the quality of almost of all codes Code Quality: Improve the quality of almost of all codes Feb 8, 2023
ferrariofilippo
ferrariofilippo previously approved these changes Feb 8, 2023
Copy link
Contributor

@ferrariofilippo ferrariofilippo left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@0x5bfa
Copy link
Member Author

0x5bfa commented Feb 8, 2023

@yaira2 Can you review?

@0x5bfa 0x5bfa changed the title Code Quality: Improve the quality of almost of all codes Code Quality: Refactor entire Files.App Feb 8, 2023
@yaira2 yaira2 changed the title Code Quality: Refactor entire Files.App Code Quality: Code cleanup an formatting Feb 8, 2023
@yaira2 yaira2 changed the title Code Quality: Code cleanup an formatting Code Quality: Code cleanup and formatting Feb 8, 2023
@0x5bfa
Copy link
Member Author

0x5bfa commented Feb 9, 2023

@yaira2 Ready for re-review

BTW, What do you think of the order of Constructors, Fields, Properties, Methods? (wont include those changes in this)

My thought:

// Constructors

// Fields and Properties

// Methods

Some codes have properties which placed above class constructor. Should have an specification.

@yaira2
Copy link
Member

yaira2 commented Feb 9, 2023

@onein528 they might need to be cleaned up a bit if things are out of order, I wouldn't go do that throughout the whole codebase now since it'll take a long time to review but I'd recommend making the change to the file in question.

@0x5bfa
Copy link
Member Author

0x5bfa commented Feb 9, 2023

Also, I have a suggestion of correcting of naming of some namespaces (wont include those changes in this)

Models:

'Views/SettingsPages/*.xaml(.cs)'        ->   'Views/Settings/*Page.xaml(.cs)'
'Views/Pages/Properties*.xaml(.cs)'      ->   'Views/Properties/*Page.xaml(.cs)'
'ViewModels/Properties/*Properties.cs'   ->   'ViewModels/Properties/*ViewModel.cs'
'ViewModels/SettingsViewModels/*.cs'     ->   'ViewModels/Settings/*.cs'
'ViewModels/Dialogs/*DialogViewModel.cs' ->   'ViewModels/Dialogs/*ViewModel.cs'


Examples:

'Views/SettingsPages/Appearance.xaml'   ->   'Views/Settings/AppearancePage.xaml'
'Views/Pages/PropertiesSecurity.xaml'   ->   'Views/Properties/SecurityPage.xaml'
'ViewModels/SettingsViewModels/AppearanceViewModel.cs' -> 'ViewModels/Settings/AppearanceViewModel.cs'
'ViewModels/Properties/SecurityProperties.cs'          -> 'ViewModels/Properties/SecurityViewModel.cs'
'ViewModels/Dialogs/CreateShortcutDialogViewModel.cs'  -> 'ViewModels/Dialogs/CreateShortcutViewModel.cs'

@0x5bfa 0x5bfa requested a review from yaira2 February 9, 2023 15:12
@yaira2
Copy link
Member

yaira2 commented Feb 9, 2023

@onein528 please let me know when you addressed a conversation and I'll resolve it from my side.

@0x5bfa
Copy link
Member Author

0x5bfa commented Feb 11, 2023

@yaira2 DONE!

@0x5bfa
Copy link
Member Author

0x5bfa commented Feb 12, 2023

OK, then. Thanks!

Copy link
Member

@yaira2 yaira2 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you

@yaira2 yaira2 added ready to merge Pull requests that are approved and ready to merge and removed needs - code review labels Feb 12, 2023
@yaira2 yaira2 merged commit 9f213a9 into files-community:main Feb 12, 2023
@0x5bfa 0x5bfa deleted the 5bfa/improve-codebase branch February 24, 2023 20:23
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
ready to merge Pull requests that are approved and ready to merge
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants