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

Added Queue problem and empty setup/solution. #19

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

Conversation

mcollina
Copy link

A levelmeup queue datatype problem for levelmeup.

The main focus is showing how easy it is to develop new datatypes on top of LevelUp.

However, I'm finding some problems in writing the setup.js to allow to require a module and execute it.
I've looked at the "horse_js_tweets" exercise, and I don't know exactly how to refactor the code to generalize the "writing modules tutorials". Maybe this feature might go into workshopper itself. Can some of you help me out on this issue?

@rvagg
Copy link
Contributor

rvagg commented Sep 16, 2013

yep, will help out with that, it's slightly awkward at the moment and needs more documentation in workshopper, looks good tho!

@mcollina
Copy link
Author

I've added a basic implementation based on level-sublevel. I'm not entirely sure that it is right to use level-sublevel here for learning purposes. What do you think?

@ralphtheninja
Copy link
Member

I think that's ok. People also need to learn that there is an ecosystem and level-sublevel is easy to grasp and you already have a hint for it.

@juliangruber
Copy link
Contributor

I think we don't want to encourage people to add level-sublevel to db automatically, as that is a huge customisation.

@mcollina
Copy link
Author

@juliangruber yeah, that's my point. But we want named queues/lists/whatever. So you think it's better just to prefix the keys, right?

@mcollina
Copy link
Author

Here is a much better implementation of the queue, it solves some issues, such as concurrent push and shifts.
https://github.com/mcollina/level-queue-type

What I think it's missing is a "know more" text file, to show after the program passes the verify step.

@rvagg
Copy link
Contributor

rvagg commented Sep 17, 2013

Just stick comments in the solution.js. You'll see already solutions with chunks of comments, like the Basics: BATCH exercise that has a whole alternative implementation within a comment block.

@juliangruber
Copy link
Contributor

leave the queue as it is, but just make it a given that db has sublevel.

@mcollina
Copy link
Author

@juliangruber thanks! done!

@rvagg
Copy link
Contributor

rvagg commented Sep 20, 2013

OK, so I've had a quick look at this and it's cool but @mcollina you need to provide the basic framework for a verification of their solution that verifies all of the things that you've outlined. For now, just make a verify.js that will load the solution and step through all of the verification steps, in a deterministic order, and prints out the status to the console. The aim should be that when you run this script across both the official solution and the user-submitted solution they should produce exactly the same stdout and that stdout should also be somewhat helpful in pointing out where problems might be for the user so they can fix stuff up.

Firstly you'd want to check that the module exports a function as you asked, then call the function and inspect the resulting object and print out the methods that you wanted and whether it has them as functions. Then perhaps put a fixed number of entries and then shift them off to make sure that it has them. Also, given that you've said that callbacks are "optional" you should check that they are indeed optional, i.e. call them without a callback and verify afterwards that the correct thing happened, otherwise you could remove the "optional" bit from the problem statement--flexibility in the problem statement does create additional overhead in the verification step.

You'll also want to consider some edge-cases to test for to see if you can catch them on things they may not have thought about.

Also, using new Date().toISOString() is going to be problematic because we'll be throwing values at them quicker than the resolution of Date, we almost need monotonic-timestamp, or tell them that they should keep a counter to put on the end of the keys, or some other solution to deal with rapid values that'll end up with the same keys for subsequent entries.

@mcollina
Copy link
Author

This is a bit old, maybe we can add this in? :)

@martinheidegger
Copy link
Contributor

@mcollina it is super-annoying, I know, but I just landed #79 which is a major refactoring and I didn't see this exercise/PR (else I would have ported that too). Do you think you can give a stab at rebasing to the new code layout?

@martinheidegger
Copy link
Contributor

@mcollina I also just read through the code. Seems like @rvagg's comment is still valid and there needs to be verification logic in this PR. It would be awesome if you could indicate in the next two days if you want to work on it. Else I am inclined to merge #81 before this and we need to wait for the japanese translation before we can merge it.

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

Successfully merging this pull request may close these issues.

5 participants