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

Issue67 #91

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

Issue67 #91

wants to merge 3 commits into from

Conversation

hlYassine
Copy link
Contributor

I'm thinking of adding others wild cards ( '?', '{ }', ...) in the future, I think I'd use the "Trie" algorithm, not sure though. Anyway, now it's only the asterisk wild card that is implemented in this commit.
I've used Singleton design pattern in the submited code, so if you don't like it I'll find another way.

@hlYassine
Copy link
Contributor Author

Sorry I found a bug on this branch. don't merge it until the next commit.
thanks

@hlYassine
Copy link
Contributor Author

done :)

@aidin36
Copy link
Owner

aidin36 commented Oct 19, 2014

Wow! Thanks for all the work!
I should take a deeper look at what you've done (:

@aidin36
Copy link
Owner

aidin36 commented Oct 20, 2014

The overal of you design is excellent. It was a good idea to have a WildCardManager, so we can support other wild cards in the future.

There's some little problems with the implementation.

  1. Why you create a Singletons class and add all the singleton classes into it? We could simply add a get_instance static method to WildCardManager. Or, WildCardManager could have just some static methods.

  2. Seems that business of WildCardManager and WildCard mixed up. WildCardManager shouldn't do somethings like checking if and expression contain a wild card char (wild_card_manager.cpp, line 39), neigther it should parse that path to split directory and file name. In this way, I can never write a WildCard that be able to process something like /path/2014-*/DSC.JPG.

WildCardManager should be only a container of WildCards. Each WildCard should have an expand method. This method will be decide if any wild card character is in the specified path, and how it should be expanded.
WildCardManager loops through the list of WildCards, and calls the expand method of them, collects the result, and returns it.

In other words: WildCardManager tells BraceWildCard: Expand this: /path/2014-{02..06}/*.JPG. It returns a list of strings like /path/2014-02/*.JPG, /path/2014-03/*.JPG, etc. Then, WildCardManager tells AsteriskWildCard: Expand this: /path/2014-02/*.JPG, it returns a list of JPGs in that directory. Then again, WildCardManager tells AsteriskWildCard to expand the second path: /path/2014-03/*.JPG. And it goes like that for each path.
If a WildCard found out that there's no wild card char in the path, it do nothing.

Encapsulation ;-)

If I couldn't explain it clearly, I can send you a UML and pseudo code.

@aidin36
Copy link
Owner

aidin36 commented Oct 20, 2014

Also, please consider formatting. Things like space after if, and no space after parenthesis, etc. Seems that you forget our coding style when you were away ;-)

@hlYassine
Copy link
Contributor Author

Thanks my friend for the remarks :). you are right.
In fact, I'm working on a design to implement all wildcards, I've seen it'll take some time this is why I submited this which lists only files.
I'll fix the formatings, sorry I mix the working code style with tocc style :D .

@aidin36
Copy link
Owner

aidin36 commented Oct 27, 2014

:D
Take your time, my firend (:

@aidin36
Copy link
Owner

aidin36 commented Nov 18, 2014

What's up, Yassine? (:

@hlYassine
Copy link
Contributor Author

hello Aidin, hope you're fine.
actually I'm still workin on the issue, I'll be able to finish it soon, then I'll refactor the code and submit it.

@aidin36
Copy link
Owner

aidin36 commented Nov 19, 2014

Thanks Yassine (: I'm waiting for that.

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.

None yet

2 participants