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

Amit Maor #5

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

Conversation

amitmaordsl
Copy link

No description provided.

@JoshuaLandgraf
Copy link

RegularExpression.scala
I see that you added a matches method that takes a Char instead of a String. I'm curious if you really needed to do this as Scala can probably convert Chars to Strings automatically and I was able to get Program.scala working without adding this method. While it doesn't hurt, I'm not sure it's necessary.

Your union, concat, star, and plus methods are all good and similar to my own. You may want to make them one-liners since they're so simple.

I noticed you used the apply method (object factory) approach to implementing the repetition operator. I swear I tried this approach, but couldn't get it to work. It's good to see you were able to as my solution was kind of crazy and nowhere near as straightforward as yours. Although, you could shorten your if/else statement a lot. You shouldn't need the curly brackets as the bodies of the statements are one-liners. Even if you did need them, you could shorten your implementation by starting the else statement on the same line that you end the if statement's body. You could also put the bodies of the statements on the same line as "if" and "else". However, these are only relatively minor details. You're implementation as a whole is good.

Nice use of reductions in your method for converting Strings to RegularExpressions. I ended up using recursion, but I think your approach is probably better.

reflection.md
The methods need to be in the abstract class because that's what the different kinds of RegularExpression inherit from. By putting the methods there, you guarantee each RegularExpression object has those methods available to it. You also have to do this because when you say "expr1 || expr2", the union method is actually being called from expr1, which is a RegularExpression. So, the method needs to be defined for RegularExpressions and not the RegularExpression companion object. Hope this helps.

I also struggled with implementing the operators that you did, but at least your solutions are pretty elegant. Also, I didn't know there were sample solutions…

I also agree that + and * could / should be simplified in the DSL. I also wrote in my own response that it would be nice if there were better symbols of the operators, like Union could be U. Unfortunately, while some more obscure characters might be nice, you'd have to worry about how users could type them easily. You definitely don't want people to have to keep copying them as that's too much of an inconvenience and will deter them.

Overall, great job. Your solutions were really nice.

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.

2 participants