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

Clean Code #67

Closed
edTheGuy00 opened this issue Feb 13, 2019 · 8 comments
Closed

Clean Code #67

edTheGuy00 opened this issue Feb 13, 2019 · 8 comments
Labels

Comments

@edTheGuy00
Copy link
Collaborator

This is not an issue but more of a proposition. I've used this library on two of my projects, great library. However, every time I look at the source code it's honestly a bit confusing.

Main Issue

There is more than 1,000 lines of code in one file, code is a bit messy, has too many nested ternary operators, makes it difficult to read and maintain.

Proposition

I propose to refactor and move out some code from the flutter_calendar_carousel.dart file into more reusable and maintainable widgets. For example,

  widget.showHeader
              ? Container(
                  margin: widget.headerMargin,
                  child: DefaultTextStyle( ........
                             .........................
                             .........................
                             .........................
                             .........................

We could move all this code into a separate widget class and just use

  widget.showHeader ? Header() : NotHeader()

The same can be done to various widgets used.

widget.weekFormat ? Calendar.weekView() : Calendar.monthView()

The project structure should look something like this.

    --flutter_calendar_carousel.dart
    --src/
       -header.dart
       -calendar.dart
       -day_widget.dart
       -default_styles.dart
        etc....

Benefits

  1. Easier to read
  2. Easier to debug
  3. Easier to maintain
  4. Testable widgets
@hyochan
Copy link
Owner

hyochan commented Feb 13, 2019

@edTheGuy00 This is absolutely great decision to go for refactoring. Everyone will appreciate your work. 👊

@hyochan
Copy link
Owner

hyochan commented Feb 15, 2019

@edTheGuy00 I've merged your PR. However, I won't close this issue yet because there seems 2 more to go from your statements.

  • Separate Header, No Header
  • Separate MonthView and WeekView

@edTheGuy00
Copy link
Collaborator Author

@hyochan Yes that's right. I'm still doing some work on it I can close this issue later on.

@KnockaertSven
Copy link

KnockaertSven commented Feb 21, 2019

This is really minor so I figured I'd put it here:
-> weekdayTextStyle is written with a lowercase d

-> the other weekday props are written with a capital D , e.g. :
• showWeekDays
• weekDayMargin
• etc

I realise this may be nitpicking, but it'd be nice if it would be streamlined :)

@edTheGuy00
Copy link
Collaborator Author

@KnockaertSven sure I'll take a look at this

@edTheGuy00
Copy link
Collaborator Author

@hyochan @KnockaertSven I think it should be lowercase d weekdays not weekDays, would you agree? I can fix that on PR #77

@hyochan
Copy link
Owner

hyochan commented Mar 1, 2019

@edTheGuy00 Agree.

@hyochan hyochan added the wip label Mar 21, 2019
@github-actions
Copy link

This issue is stale because it has been open 90 days with no activity. Leave a comment or this will be closed in 7 days.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

3 participants