Skip to content
This repository has been archived by the owner on Dec 19, 2021. It is now read-only.

AOC-024: Day 7: The Sum of Its Parts #71

Open
wants to merge 18 commits into
base: master
Choose a base branch
from
Open

AOC-024: Day 7: The Sum of Its Parts #71

wants to merge 18 commits into from

Conversation

jpgoelz
Copy link
Owner

@jpgoelz jpgoelz commented May 22, 2019

Day 07 has been completed!

@jpgoelz
Copy link
Owner Author

jpgoelz commented May 22, 2019

@jpgoelz jpgoelz requested a review from mfbieber May 22, 2019 08:28
@jpgoelz jpgoelz added Backend Everything concerning the backend Story Days and Parts of the Advent of Code. labels May 25, 2019
*
* @return time it takes to complete all the tasks
*/
private int determineTime() {
Copy link
Collaborator

Choose a reason for hiding this comment

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

This of course is like a "doMagic()" method. Although I am sure that it does what it is supposed to do, it is way to long for somebody to understand who is reading the code for the first time. Not because it is not good to read and not well structured, only because of the underlying complexity of what it does.

Also, consider what would happen if we refactored it? The test would break if we make a mistake, but we would not have a clue about why - or do we (also because we don't test it since it is a private method=?

To improve this, I would attempt to make it testable. For that, maybe moving it to a new class would be the first step, and splitting it into smaller, simpler methods. Then testing each of them in the new class.

If you feel that you have to test a private method, it has gotten too complex/large and something is wrong by design.

Copy link
Owner Author

Choose a reason for hiding this comment

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

You are right.. hmpfgrmbl. ;)

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Backend Everything concerning the backend Story Days and Parts of the Advent of Code.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants