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

Add Two Bucket Exercise #636

Merged
merged 6 commits into from
Feb 25, 2024
Merged

Conversation

tomasnorre
Copy link
Contributor

We didn't agree on this, either. But here it is. Feel a little like a rebel not following orders.

http://forum.exercism.org/t/implement-some-missing-php-exercises-for-48in24/9865/11

@mk-mxp
Copy link
Contributor

mk-mxp commented Feb 22, 2024

We didn't agree on this, either. But here it is. Feel a little like a rebel not following orders.

Is that a good feeling to you? You do not have to apologize for the many useful work you contribute. You are welcome!

@tomasnorre
Copy link
Contributor Author

We didn't agree on this, either. But here it is. Feel a little like a rebel not following orders.

Is that a good feeling to you? You do not have to apologize for the many useful work you contribute. You are welcome!

Am not feeling good or bad, just a feeling, that I'm not really following the normal process of asking before doing.

Verified

This commit was created on GitHub.com and signed with GitHub’s verified signature.
@tomasnorre tomasnorre force-pushed the add-two-bucket-exercise branch from da06e94 to 268a243 Compare February 23, 2024 13:05
@mk-mxp mk-mxp added x:action/create Work on something from scratch x:knowledge/elementary Little Exercism knowledge required x:module/practice-exercise Work on Practice Exercises x:size/large Large amount of work x:rep/large Large amount of reputation labels Feb 23, 2024
exercises/practice/two-bucket/.meta/example.php Outdated Show resolved Hide resolved
exercises/practice/two-bucket/TwoBucket.php Show resolved Hide resolved
Copy link
Contributor

@mk-mxp mk-mxp left a comment

Choose a reason for hiding this comment

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

Sorry, wrong selection.

Comment on lines 29 to 37
public function __construct(int $sizeBucketOne, int $sizeBucketTwo, int $goal, string $startBucket)
{
throw new \BadMethodCallException(sprintf('Implement the %s method', __FUNCTION__));
}

public function solve()
{
throw new \BadMethodCallException(sprintf('Implement the %s method', __FUNCTION__));
}
Copy link
Collaborator

@neenjaw neenjaw Feb 24, 2024

Choose a reason for hiding this comment

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

This interface is an improvement from the last, but since we don't have a meaning for what the buckets mean before the solve method is called, I think we should take one of these two options:

// Option A

class TwoBucket
{
    // drop the constructor, class properties

    public function solve(int $sizeBucketOne, int $sizeBucketTwo, int $goal, string $startBucket)
    {
        // perform the solution for the parameters provided, return a solution object
    }
}

// or Option B

class TwoBucket
{
    public function __construct(int $sizeBucketOne, int $sizeBucketTwo, int $goal, string $startBucket)
    {
        // initialize and solve the solution, making the buckets available in getters
    }

    // drop the solve method
}

Currently we have an object that requires its public methods to be called in a specific order before it is considered valid -- this is a cumbersome pattern that shifts the burden of implementation to the consumers of this object to understand the inner state of the object to use it correctly rather than the object being the expert on itself.

Copy link
Contributor

Choose a reason for hiding this comment

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

Thanks for pointing that out. It is inherently a procedural problem, "input, process, output". Which means, the only feasable way to implement it is just a single function or method call.

Copy link
Contributor Author

@tomasnorre tomasnorre Feb 24, 2024

Choose a reason for hiding this comment

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

I opt for option A. I like that approach more than having the logic in the constructor. For me, the constructor is for setting up the Class/Object, not doing any logic.

I think I'll manage to have it adjusted one of the following days, if neither of you opt against it.

@mk-mxp mk-mxp merged commit 2efa3a3 into exercism:main Feb 25, 2024
12 checks passed
@mk-mxp
Copy link
Contributor

mk-mxp commented Feb 25, 2024

Thanks a lot for contributing! It took a bit longer to figure it out, but we are both learning about what's relevant for a good practice exercise.

@tomasnorre
Copy link
Contributor Author

Thanks a lot for contributing! It took a bit longer to figure it out, but we are both learning about what's relevant for a good practice exercise.

You're welcome. It doesn't really matter how long it takes, as long as we keep the eye on the target and stay constructive in feedback and dialogue.

I really appreciate inputs from both of you @mk-mxp and @neenjaw.

I hope I can continue to bring quality work to the PHP track.

@tomasnorre tomasnorre deleted the add-two-bucket-exercise branch February 25, 2024 20:18
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
x:action/create Work on something from scratch x:knowledge/elementary Little Exercism knowledge required x:module/practice-exercise Work on Practice Exercises x:rep/large Large amount of reputation x:size/large Large amount of work
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants