Thanks Alex. This looks like excellent feedback. I'll digest this and report
back.
Jon
On Thursday, October 10, 2013 12:20 AM, Alex Vondrak <ajvond...@gmail.com>
wrote:
Overall, the code is readable. Nice work!
It's a bit odd to have a separate private.factor file on its own. The
only Factor project to do this (in my git pull from July) is
unmaintained:
$ find . -name private.factor
./unmaintained/alien/marshall/private/private.factor
More common is to use `<PRIVATE ... PRIVATE>` syntax in the same file:
$ grep -l "<PRIVATE" **/*.factor | wc -l
613
See http://docs.factorcode.org/content/word-__lt__PRIVATE,syntax.html
A few things I noticed (mind you, it's late at night for me right now):
https://bitbucket.org/jherron/market-movers/src/tip/yahoo/finance/market-movers/market-movers.factor
All around, you use a lot of `{ } nsequence`. For one, you can use
http://docs.factorcode.org/content/word-1array,arrays.html For
another, you're defining words to calculate constants. Consider
instead using the literals vocab:
http://docs.factorcode.org/content/article-literals.html
https://bitbucket.org/jherron/market-movers/src/tip/yahoo/finance/market-movers/market-movers.factor?at=default#cl-38
Consider using http://docs.factorcode.org/content/word-C__colon__,syntax.html
https://bitbucket.org/jherron/market-movers/src/tip/yahoo/finance/market-movers/private/private.factor
All around, you seem to have some long lines. The style convention is
to keep source code 64 characters wide:
http://docs.factorcode.org/content/word-margin,prettyprint.config.html
https://bitbucket.org/jherron/market-movers/src/tip/yahoo/finance/market-movers/private/private.factor?at=default#cl-12
Couldn't this be a `CONSTANT:`? I mean, if you even want to bother
defining it as such, instead of just inlining the regular expression
into `extract-symbols`.
https://bitbucket.org/jherron/market-movers/src/tip/yahoo/finance/market-movers/private/private.factor?at=default#cl-14
Don't worry, the length is constant-folded by the compiler already:
IN: scratchpad USE: compiler.tree.debugger
IN: scratchpad [ [ "q?s=" length + ] 2dip ] optimized.
[ "COMPLEX SHUFFLE" 4 + "COMPLEX SHUFFLE" ]
That said, I think code like
USE: splitting
"q?s=FB\">"
"q?s=" ?head drop
"\">" ?tail drop
might be clearer. But it's a minor point, and you have to balance
that with the fact that `map-matches` is already giving you input
suitable for `subseq`, so you might as well just use that (like you're
doing already).
https://bitbucket.org/jherron/market-movers/src/tip/yahoo/finance/market-movers/private/private.factor?at=default#cl-26
product-sequences already implement the sequences protocol; I doubt
you need to call `>array`.
https://bitbucket.org/jherron/market-movers/src/tip/yahoo/finance/market-movers/private/private.factor?at=default#cl-28
Consider using http://docs.factorcode.org/content/word-map-sum%2Csequences.html
https://bitbucket.org/jherron/market-movers/src/tip/yahoo/finance/market-movers/private/private.factor?at=default#cl-30
You could simplify the logic slightly, if you're so inclined:
: symbol-locations ( assoc key -- hash-set )
swap [ [ HS{ } clone or ] change-at ] [ at ] 2bi ;
Oh, by the way, `assoc` is a more typical stack-effect name than
`hashtable`. See
http://docs.factorcode.org/content/article-conventions.html
https://bitbucket.org/jherron/market-movers/src/tip/yahoo/finance/market-movers/private/private.factor?at=default#cl-34
`[ p ] keep q` is the same as `[ p ] [ q ] bi`. Here, `[ category>> ]
[ group>> ] bi`.
https://bitbucket.org/jherron/market-movers/src/tip/yahoo/finance/market-movers/private/private.factor?at=default#cl-37
In general, I find `reduce` hard to read. It looks like you're using
it to try to preserve & return the hashtable at the end. I'm not
sure, and I won't hazard to rewrite the logic myself in my current
state, but you might be able to just pass around a reference to the
hashtable in a more conventional iteration:
IN: scratchpad H{ } clone { 1 2 3 } over '[ dup 100 * _ set-at ] each .
H{ { 100 1 } { 200 2 } { 300 3 } }
Hope that helps & happy hacking,
--Alex Vondrak
On Wed, Oct 9, 2013 at 8:10 PM, Jon Herron
<jonhdispatch01-fac...@yahoo.com> wrote:
> That would be very cool. I'd love to contribute if anyone else would find it
> helpful.
>
> The repo is @ https://bitbucket.org/jherron/market-movers
>
> Thanks for taking the time.
>
> Jon
>
>
> On Wednesday, October 9, 2013 8:05 PM, John Benediktsson <mrj...@gmail.com>
> wrote:
> Sure, send a link to your code. We'd be happy to review. Also, I have some
> support for getting quotes from Yahoo! Finance - maybe you could contribute
> some of your work to the project:
>
> https://github.com/mrjbq7/re-factor/blob/master/yahoo/finance/finance.factor
>
>
>
> On Oct 9, 2013, at 7:53 PM, Jon Herron <jonhdispatch01-fac...@yahoo.com>
> wrote:
>
> Hi all -
>
> In my spare time I am working on a backtesting application and I've
> decided to write it in Factor. So far all I have is a vocab for
> scraping/parsing the market movers from Yahoo Finance (eg
> http://finance.yahoo.com/actives?e=us). Before I write too much more Factor
> I was wondering if anyone more experienced would mind reviewing my code? I
> feel pretty confident that the code works, I just don't know if it is "good
> Factor". If this is an OK request I'll send the link to my repo (I don't
> want to come across as spamming on my first post).
>
> Thanks,
>
> Jon
>
> ------------------------------------------------------------------------------
> October Webinars: Code for Performance
> Free Intel webinars can help you accelerate application performance.
> Explore tips for MPI, OpenMP, advanced profiling, and more. Get the most
> from
> the latest Intel processors and coprocessors. See abstracts and register >
> http://pubads.g.doubleclick.net/gampad/clk?id=60134071&iu=/4140/ostg.clktrk
>
> _______________________________________________
> Factor-talk mailing list
> Factor-talk@lists.sourceforge.net
> https://lists.sourceforge.net/lists/listinfo/factor-talk
>
>
>
>
> ------------------------------------------------------------------------------
> October Webinars: Code for Performance
> Free Intel webinars can help you accelerate application performance.
> Explore tips for MPI, OpenMP, advanced profiling, and more. Get the most
> from
> the latest Intel processors and coprocessors. See abstracts and register >
> http://pubads.g.doubleclick.net/gampad/clk?id=60134071&iu=/4140/ostg.clktrk
> _______________________________________________
> Factor-talk mailing list
> Factor-talk@lists.sourceforge.net
> https://lists.sourceforge.net/lists/listinfo/factor-talk
>------------------------------------------------------------------------------
October Webinars: Code for Performance
Free Intel webinars can help you accelerate application performance.
Explore tips for MPI, OpenMP, advanced profiling, and more. Get the most from
the latest Intel processors and coprocessors. See abstracts and register >
http://pubads.g.doubleclick.net/gampad/clk?id=60134071&iu=/4140/ostg.clktrk
_______________________________________________
Factor-talk mailing list
Factor-talk@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/factor-talk
