From 9542b8ed838c49819602ecc6d74e49d07256248b Mon Sep 17 00:00:00 2001 From: dgbillotte Date: Mon, 22 Nov 2010 17:20:15 -0700 Subject: [PATCH 1/2] job application by daniel@litfuze.com --- github-challenge/github_rails_commits_yaml.rb | 31 ++++ refactor-this/REFACTOR_README.txt | 40 +++++ refactor-this/helper_notated.rb | 92 +++++++++++ refactor-this/helper_spec_notated.rb | 149 ++++++++++++++++++ resume/daniel-resume-2010-11-11.doc | Bin 0 -> 39424 bytes 5 files changed, 312 insertions(+) create mode 100644 github-challenge/github_rails_commits_yaml.rb create mode 100644 refactor-this/REFACTOR_README.txt create mode 100644 refactor-this/helper_notated.rb create mode 100644 refactor-this/helper_spec_notated.rb create mode 100644 resume/daniel-resume-2010-11-11.doc diff --git a/github-challenge/github_rails_commits_yaml.rb b/github-challenge/github_rails_commits_yaml.rb new file mode 100644 index 0000000..ff9c122 --- /dev/null +++ b/github-challenge/github_rails_commits_yaml.rb @@ -0,0 +1,31 @@ +require 'YAML' +require 'net/http' + +commits_as_yaml = + Net::HTTP.get('github.com', '/api/v2/yaml/commits/list/rails/rails/master'); + +tree = YAML::load(commits_as_yaml) + +authors = {} + +tree["commits"].each {|c| + author = c["author"] + login = author["login"] + unless(authors[login]) + authors[login] = {:name => author["name"], :commits => []} + end + + authors[login][:commits].push({:id => c["id"], :message => c["message"]}) +} + +puts "Recent Commits to rails/rails on GitHub" +puts "" + +authors.each {|login,info| + puts "

" + info[:name] + "

\n" +} +puts "" diff --git a/refactor-this/REFACTOR_README.txt b/refactor-this/REFACTOR_README.txt new file mode 100644 index 0000000..5acdaf2 --- /dev/null +++ b/refactor-this/REFACTOR_README.txt @@ -0,0 +1,40 @@ +While getting the tests to run and refactoring the code there are a +number of assumptions that I had to make. These are: +- The tests are authoritative and take precedence over the code +- Any code or requires that were not used by the tests were not needed and could + be removed +- Since the assignment here is to “make the tests work and refactor the code” + rather than “make the code work”, I added stub! statements to the tests to + make up for any functionality missing from the main code even if the + additions might have been trivial +- I noticed that many of the missing methods are available in the context of a + Rails application. At first I tried to find a way to run the tests in the + context of a Rails application so that these methods would be available, but + started thinking that that was outside the scope of the assignment and + decided to use the stub! statements instead. The fact that all the tests + tested against strings like “just image” instead of real paths, URLs, or HTML + tags helped me to solidify my opinion that this was the best way to go + +Given the information I was provided with and the lack of context that comes +with that, I saw two main tasks for refactoring here: +1) get rid of unused or duplicated code +2) clean up the redundancy in the class structure + +Getting rid of unused or duplicated code was easy. With the class structure, +again due to lack of context, I had to make some judgment calls. It seems to me +that a user and a profile are essentially the same thing. I saw nothing to +indicate that a profile might have more than one user or that a user could be +used without a profile. Given that, I removed the User class and updated +helper.rb and helper_spec.rb to reflect those changes. Factory Girl and the +Profile class aren’t used by the tests so I removed all references to those with +no other changes needed. + +The two files that I made major changes to are helper.rb and helper_spec.rb. As +I made changes I put in comments to describe why each change was made. This made +the code hard to read while refactoring so I saved the commented versions as +helper_notated.rb and helper_spec_notated.rb and then removed most of the +comments in the original files. As I refactored I tried to keep the two copies +in sync and am submitting the notated versions in case you would like more +insight into the process which I used in this assignment. That said, there could +be slight variations between the notated and non-notated versions, but both +versions run correctly for all the tests. diff --git a/refactor-this/helper_notated.rb b/refactor-this/helper_notated.rb new file mode 100644 index 0000000..50d90dd --- /dev/null +++ b/refactor-this/helper_notated.rb @@ -0,0 +1,92 @@ +class Helper +=begin +NOTE: Removed because these methods are not called by the unit tests or other code + def self.foo + "foo" + end + + def image_size(profile, non_rep_size) + if profile.user.rep? + '190x114' + else + non_rep_size + end + end + + def display_small_photo(profile, html = {}, options = {}) + display_photo(profile, image_size(profile, "32x32"), html, options) + end + + def display_medium_photo(profile, html = {}, options = {}) + display_photo(profile, image_size(profile, "48x48"), html, options) + end + + def display_large_photo(profile, html = {}, options = {}, link = true) + display_photo(profile, image_size(profile, "64x64"), html, options, link) + end + + def display_huge_photo(profile, html = {}, options = {}, link = true) + display_photo(profile, image_size(profile, "200x200"), html, options, link) + end +=end + + def display_photo(profile, size, html = {}, options = {}, link = true) + # NOTE: if this "should not happen" why is it here and why is there a unit test for it??? + return image_tag("wrench.png") unless profile # this should not happen + + show_default_image = !(options[:show_default] == false) + # NOTE: removed becase this line has no effect on the unit tests + #html.reverse_merge!(:class => 'thumbnail', :size => size, :title => "Link to #{profile.name}") + # NOTE: existence of profile has already been tested above and User has been refactored out + #if profile #&& profile.user + # NOTE: added call to to_s to get the photo filename. More would be needed here to + # get the file's correct path, but this isn't tested, so... + # NOTE2: refactored out the User class + # if profile.user && profile.user.photo && File.exists?(profile.user.photo.to_s) + # NOTE3: with User gone, I decided to use the has_valid_photo? method in UserProfile + #if profile.photo && File.exists?(profile.photo.to_s) + if profile.has_valid_photo? + # @user = profile.user + if link + #return link_to(image_tag(url_for_file_column("user", "photo", size), html), profile_path(profile) ) + return link_to() + else + #return image_tag(url_for_file_column("user", "photo", size), html) + return image_tag() + end +=begin +NOTE: removed becase this code creates a value that isn't returned, isn't assigned to a variable, and doesn't have any side-effects and, therefore, doesn't do anything + else + show_default_image ? default_photo(profile, size, {}, link) : '' +=end + end + #end + + show_default_image ? default_photo(profile, size, {}, link) : 'NO DEFAULT' + end + + def default_photo(profile, size, html={}, link = true) + if link + # NOTE: refactored out the User class + # NOTE2: after further refactoring, since link_to is alreayd mocked to provide + # the appropriate value, the call to rep? becomes useless at this stage + #if profile.rep? + #link_to(image_tag("user190x119.jpg", html), profile_path(profile) ) + link_to() + #else + #link_to(image_tag("user#{size}.jpg", html), profile_path(profile) ) + #link_to() + #end +=begin +NOTE: removed because no test exercises this code path + else + if profile.user && profile.user.rep? + image_tag("user190x119.jpg", html) + else + image_tag("user#{size}.jpg", html) + end +=end + end + end +end + diff --git a/refactor-this/helper_spec_notated.rb b/refactor-this/helper_spec_notated.rb new file mode 100644 index 0000000..5c625ca --- /dev/null +++ b/refactor-this/helper_spec_notated.rb @@ -0,0 +1,149 @@ +# NOTE: removed requires because they are not needed to pass the unit tests +#require 'rubygems' +#require 'factory_girl' +#require 'factories' +#require 'rspec' +#require 'rspec/autorun' +#require 'redgreen' +require 'user_profile' +require 'helper_notated' +# NOTE: refactored out the User class +#require 'user' +require 'photo' + + +describe "Helper" do + before(:each) do + @helper = Helper.new + @helper.stub!(:image_tag).and_return("wrench.png") + end + describe "display_photo" do + it "should return the wrench if there is no profile" do + @helper.display_photo(nil, "100x100", {}, {}, true).should == "wrench.png" + end + + + describe "With a profile, user and photo requesting a link" do + before(:each) do + @profile = UserProfile.new + @profile.name = "Clayton" +=begin +NOTE: refactored out the User class + @user = User.new + @profile.user = @user +=end + @photo = Photo.new + @profile.photo = @photo + @profile.stub!(:has_valid_photo?).and_return(true) + + @helper.stub!(:link_to).and_return("this link") + end + it "should return a link" do + @helper.display_photo(@profile, "100x100", {}, {}, true).should == "this link" + end + end + + describe "With a profile, user and photo not requesting a link" do + before(:each) do + @profile = UserProfile.new + @profile.name = "Clayton" +=begin +NOTE: refactored out the User class + @user = User.new + @profile.user = @user +=end + @photo = Photo.new + @profile.photo = @photo + @profile.stub!(:has_valid_photo?).and_return(true) + @helper.stub!(:image_tag).and_return("just image") + end + it "should just an image" do + @helper.display_photo(@profile, "100x100", {}, {}, false).should == "just image" + end + end + + describe "Without a user, but requesting a link" do + before(:each) do + @profile = UserProfile.new + @profile.name = "Clayton" + @helper.stub!(:link_to).and_return("default link 100x100") + end + it "return a default" do + @helper.display_photo(@profile, "100x100", {}, {}, true).should == "default link 100x100" + end + end + + describe "When the user doesn't have a photo" do + before(:each) do + @profile = UserProfile.new + @profile.name = "Clayton" +=begin +NOTE: refactored out the User class + @user = User.new + @profile.user = @user +=end + @profile.stub!(:has_valid_photo?).and_return(false) + end + describe "With a rep user" do + before(:each) do + # NOTE: refactored out the User class + @profile.stub!(:rep?).and_return(true) + @helper.stub!(:link_to).and_return("default link 190x119") + end + it "return a default link" do + @helper.display_photo(@profile, "100x100", {}, {}, true).should == "default link 190x119" + end + + end + + describe "With a regular user" do + before(:each) do + # NOTE: refactored out the User class + @profile.stub!(:rep?).and_return(false) + @helper.stub!(:link_to).and_return("default link 100x100") + end + it "return a default link" do + @helper.display_photo(@profile, "100x100", {}, {}, true).should == "default link 100x100" + end + end + end + + describe "When the user doesn't have a photo and we don't want to display the default" do + before(:each) do + @profile = UserProfile.new + @profile.name = "Clayton" +=begin +NOTE: refactored out the User class + @user = User.new + @profile.user = @user +=end + @profile.stub!(:has_valid_photo?).and_return(false) + end + describe "With a rep user" do + before(:each) do + # NOTE: refactored out the User class + @profile.stub!(:rep?).and_return(true) + end + it "return a default link" do + @helper.display_photo(@profile, "100x100", {}, {:show_default => false}, true).should == "NO DEFAULT" + end + + end + + describe "With a regular user" do + before(:each) do + # NOTE: refactored out the User class + @profile.stub!(:rep?).and_return(false) + end + it "return a default link" do + # added '{:show_default => false}' and changed expected result to + # "NO DEFAULT" because the outer describe statement says: + # "we don't want to display the default" + @helper.display_photo(@profile, "100x100", {}, {:show_default => false}, true).should == "NO DEFAULT" + end + end + + end + + end +end diff --git a/resume/daniel-resume-2010-11-11.doc b/resume/daniel-resume-2010-11-11.doc new file mode 100644 index 0000000000000000000000000000000000000000..12d3c352437aed1e6776e52a3fe48bd5f5e04ed7 GIT binary patch literal 39424 zcmeHw30zcF`~R6?RY4FH$@Pk;sO&1DxqyHIilX3_OAIp?7#(KDSx_tsw={iwB`vdZ zU&<_(G?z3@(_AVQ&0NxQNlQ%&%Um+$|9#G#3&RLX%iH_@`*Hbv=iYPfInO!g+0Js$ zy%Zj*b$0Q)_0KVfqbqY__lv7BSG#xxt`p?2H)Bb-rnvjX#l;le6ToHF@&6GAt}frn zobszVGj`kOLlPomkSq>&#;!MFtR~ACnK3f$nY3qkY?+^G&b}<99b=Jo6dc#H1;kI&yZLNeC45dQ6He?;L8QTc8 zHXwck;t$yJS9D%cpRshLb8o;{L!1L}CKd*OjuPhc%e52UwNoWpP?{iEZkz?cWly;@KR`UT}m162x>DV!B3_OCnSa%76K zyr>+im*eqg9{3|F>%nZDy$=CfnGV^}U%mapUaQTH9w|5agGUXJyB zR9LR>?>$#UmwaoVAP31rpZ7AM6F3MW*hXPIf#m)I*buBf>4}B$Pn`k zdQFIDtjdH^h15|Hx;eC$-Y`ZdYO=&mo{2)X3e6Z}FpUyXLzFC-4VjiPNMBM}sn84q zO3qNJM+J)@9H>!QR2eEWbOdds>;kx!W7W&G4iYE@DxuQlS+r`@O$ujanZzvKupq%K zs!ZzaAVIXKLt63rqDguSF91ydht%16gU*np70rTKOH2;$*fBCh@a(P9XIWKQqPdfh zq{>y9)h4acA_O9d&Z;5iTFs(h$mGmOF&QidOCG6noMEIPNN6D>^|OjxFb3Tyt;O6b zNJvRY2~EvWnJjrhoMLEFYD$pM(_}E@=7sj8>)2*NLVs%p?x%<*9m1ipZQ2A0-Bo6^ zfOxGEppzEJB|~eWvdv&<3qdtzRETHtz*tC2l_L_P%t$v%6#Bz%%%MF^bb_I!ri3C@ zkdSIISuKc2hFDBmH8RK9I4x*Ivo=c~BxrLW{Txwmv9VDLjmt9e02h^19q47I!IYyS zu2J>WDx*rRwLp2z7HtkvgG&@|fy{7;v_f)TYQNs7LciWZs%S#_5+w*8x?>V`ya<(! zOOj-74)GjFd@MBrjif7m`+_tCZ`P9BZPlj2j3xtWnv;VPrG|y}pgLN3ZR~`ake1v# zG&VIA!bt7gCrB8ajCu`@pID%|sFbf)(FwucmP-#S{Gdu&;L81!}V&a0* zi(|?fh67*GW}9t<2#J|OBGH{?5k|r01+66r5hx%V4n}rNCa}X2;wi{ZB1WC96?3_Q znLByLQX|MUkbOh%ED}3O{E}LkL=E&^tumEiREeVi$H#~|9dVvKQ;8Mu%sDV{^c!lk zY^$aA9|(&Sc%o7RZ^gB=tsn-IuMthqY_rw^-#}vNW5|W$?5)idg;awM&Kj;WNa)== z7K+9-kQxDYlC!n&x{Ner2&z(lWRoQ)mP$63m!L)Cs4El3WNTsAFeVlJH|$1o-Z1j; zuo5?khGrXVLIvZ+$P75yrt>yZp$u*#_LQ=>9`x9vPgHyG9+(dKvOay0F&0BL1n-NgGo{( z^4&o&LaUz6W;lO17EVCah$b`${i+o%+5oqQo>8wxry1heoolLi5piBAiqp+>3K69Q@20txWJ3dSm_ zS{>@GLpKC>F87q42?+BzIPGA;#%k_HN|`OO5Caw7Ekb^9YP=sT9d1IJG;q-v)K(f| zNIs8u2C{$TRw|DvF&+2ufCRpfH)m}Sm3Kv2B%ROOQW8!-ZgTIZmn!)VU4 zLf?%cm{te5prPPwj5P}*YLZbDSevZM<6L%gi-Vt5L-9#DWWJ&l2iBrPDa+Ord0=SA zTUa5wvtWnr48t)R8aeP-wk-$!x3$wYDyfK|!33)nGOfBy49SVpk}<2%Km+VWFk6je z`XP{|R89`G9C9Ohk<*uk$u{nj|Hu|~Ms6Kr47MQ`beq?ut)RDBOj_y@Ar`m@$k%2x z!Dgrcjjhp(Qc(%8KJutUFh@1gfLwV*G%*bxMATrooelR4-qPTmMu?&EJb*k!rZ&qe zjYFvyFzO6>)G+iBaLAa~(5%Nce)E)iK7seJ=tK1C5b&6Mn+nc+pjI4%0d|aAl1gtS zM+Awths5pVU$H?6p|DL1agwRC7EKOHU_XK<6XQcmnHykGC>UcKLE41Ql^VjAN5nK7 z7h>dTje|-^h0bg)3Wz&mW-ne*9E$zYqrQ)VE; zI}FGQGd4a5rEbWK{t9X$c~O%Uj+O{!N`nAtOcK#Oye{?NrA46=(NWJpLu5XFqhSHH3kDr5B+7FXUP0d6lEF4+n9}%6>icXXY1E#& zdQxx506n0EW_{EfAV_^Hv{LF^DNdzUld7R3wPmLwcxt;YljzCaG!!yd)ESJDsd2TJ zEKVFNsxex`n2Yzx#G3xRCpDoTr}-KR<}QwS13Tb?MP<3YsjRS8s)kK%XbxWD8U>9u zGgCB?7d2|p!Ei3gv+E!^3$HZi4LU!`F+&ZA5VezhCO1Ubm(Bq5En{Y|SPPPebCv<9b5!V$=5M_=tx_TwKX$Ol5O=7*)wxN%qjcY}GhSEXoeVh%IJo21Y>W(#=?^ zf?RBr$6z$0%7FSqK9YHW2&QRTK0K0!QdBy!=V&oaghNEiTop!JG}ik=BdlN{me2EO zRKr`FtL21I``DN`=zxl5ypkuD#A{m^qDo2iwy8NLZWiu^;YISe8e#CjXT~rsJ^&>Z z%;a*>7>Ie?uFNXf8bWZ%M)DtOOcnK3qohBS15+$MYe9|d#)!0*K>8yO>sk|X=$v_V6;f{wdBtq3noV1)X1@71!+Rv&T zHx8y_YZ=7rxFn6|ytRQSARg!e=zttR4_JU)U>-0ZSO6>p76W^MeZYR85I6$d2JQgA z0(XIXK(lIi8VWQA0)bXQ8z2a{a$?W+Eo-(f-@fHF`k6^Tlj$c9Kck*-tSt{W_09gM zvlDCYKB0SSMc*VP>jw;TpD+R^V>TtJFQtf0@@4XUx~W2#)m6Gra2oj%p0K*QE2MM- z+!a#nk9czNU~E#kvCC18eOe!hR#;N)k}_>&QH|_Vx=JaTm(xhPRZ@p$(iIw2Ud5%B z*{AU-DM)IZkFE8sBsvm*Vv_<&lG%8qLX<>}HKFJH_VSnI5(U}Z0=%n3&Ok1n@~j6^ z@TBJ!)mlEjlP~Ce#y)V=K2TsEm}(!GXdlS459IuTKxq~ae^$K(tIb3y=={3m61vG8o7Q76J8ZVh02e4x|A_;NRHtF&{XAEfJ>)VAfZ_ zC7^k2?1}(708zk|)1O{Bz0LNwhM(tOIX(0A1pJM^a$0v~KC!5B$D@6OY=sScv(NQk z$Lw55sUN|;q_pGZ-?O*G2i=ThU&!zN#Gdl=<6c7G!1C`kt;_S3_yc<Bd^ zklRA%2FJ<-8Y5AV8M;EL^{g?MQQgcY$rdQQ#WzJhq|D1a1H~ zfvFfxO#|iutAYK%PXMcjolAfk+mtlGVeA|`0$c-b0d=u6ts@W*EXJ;=C4d)pH+ciK z03SdAB7iH$cCT4HlmAXwY<9ewxmd&>ZllnL`BU6`n2-0jPAcD%R?^2;))SX@6D1=t zUz;bkFR7w&U5k?bAU3Hq&2VYpt3s4*P-pZ1k$ij{n^fxmOOlrO|CVIuB=awUZ|!6+ z%l!+R?7KqVNx(Xw5o9dOUY7f>T%Bz{l??o1pXZ8w;Iw_qi>y1r=RxZ>mWi{o?Z)( zZ9mxTaNq;L9d?@uG=sg$_AA@&CVPRb`%WBGT#Mpe%I)#~ zD_Z11zStL#{c(Tz;-!G>hsg)00DrB_|J%a+1Df&I$^Yp8ko>F3@*N2o*Tqh0U*HGe zN1zCxcL1)P_~Zlpt^5OA`#+h*zArEB^5q4nlKESOvpV1~vip zAagyi29R~&ueJY5b{hW_?J1duasRJ&{onEb)q+@fLAIjF+QLv_?b$9(*DiAL2bvWu zatQ#MuZ1?OEmZme9tBuJt2+K+9H3R~xIf#b!u?nB?rb?&>t7SuO4jSG8l)$*W_i{j zz8&Inn2Kqcj#-$2bz&NX^azWHi3YTY&tz6M4lyB2jWjvPQ&q_-PC^ZS*YTg$$pK|n zt(xXCWwxUIF+6B+O7Ov1p~SfvViSBi4Xhz8;MBW`7El_x7kaD)Q(``em%s2}z_t*D zwU;tv6?3K)gS3-OI?O{N&Cg0Fr@PXRxGt}t(v&Dr>EuO=LGCD!7q2K2QkDrh z%OSor_Ck5pDrxZrU#VJ0Jy9LS1>a`53guDH2dkrSDou$BD3oQYqbO4yMVaa-eou8| z0T@XD3KD}tX)H$!rJ^#DII5~ckElwN^qKB%SQ>IUumm*I=dhGQ}K0{Zr7G-Y!LpUkjDx>g=0Z940I#0>6t~kvkva8YiruFB5xE< zvF(CRZH&fvu#Y2;86jB{Uu@R1QQ&Gr zFjLXJb+soGilKGId>c(ZGvmqvMDmu!C?f6XxUueB$4EykXa{^0Hd#8c6rdk40_el; zdns6kqEDH83CN*B+qK|B7V>$rx=h-KJeIj~8<6*U#wIlcJ!jBz!N&E5s7H77(-~#z zQ4MhhP|Jg+rh)QU&cW);7c^m)%f_OFKo-hW3X>wxt{%>1w6GqNN=ab;C}E-^!Om9Q zSS;#m1YIq+8>+D%sE_5;Jy2sGZncr_3aO%g&^|>7Z~H8qORdv|C34wQEy?B|W0YGe zxjVaWxL&+O(cLb+8`qaSZim6J0LoLJ`-wW>i?YPy$UZ`%vF;%>A~=l(kEHR@Luj<+ zG#WmV#*v56pvNFCtkENBoPP+7_MArJN7DG|Av7X6jV6zzp;T5ZmkykU@JJdy51|po zX*7KZjUHS-{FP;GS@xvlS0p_lzWP6ej-(?I52;H>UYBMMq0yP^c6ViGJ6)|=rq77A z^)MpzUGfetEP!h}-mgLK7-j!5E%7Xq+tEYRV9gnJ1!E^L*^4WB0L3tOMM^Ix=Q;?{ zIK3&GIQ@g#wB@ZKdSh(jV?$%{J}_Rndom55xiJq98JnYf=fzJ)?h5mczcuClq{59S zOj`n%pE$TAam@IjBVK+N-VGi(b8@?C`_GKIx;bTwd$6UMcgDe2pBk`w<~NIK`py11 z?ec&T*;6#*zghA2jk`az+IqZoc#6}cmharCHhaYJ|YRH=J;>{^PZGJQF?j+l_%g{Ogz93G=S+F0|fhT_f)JqH&Yh+oJno8D7>`{Al?3VQVcPPq)L zmLGfbyB%OrB<@mI{UqLiHiw4L@8p?>z#@;SKTg>;>G1e|M|Sx&&kbE(5F5I5JUbWo zY_@-cQw_d9)pxIJ^{wucm;HF_tuFUh&$x5$rSAma*H*gjIv?BZ)9iD(T|Rtv;{GYi z-fZqZbgc2YrB9vO@Z<+6hnIFf5Z+*U!p5+deY~cf?E69g)!j9R_j?~nia*sZBjDLL zUmX49@g{Q@Ja#s+_vgR(_CMYsef7*0`-|7_3j1t9-<{8{3Az~XtnctrZ|#MQ#VbO; zN!?bkLR@*Yap8u|caGgFx1po*=J-xR1E&Mn)WPs#SdJKK_iKA0nv}!iv*hZ#1n=l` z;}?%9IN3qHH84J7-KVR!-ej@!>c{L|bY$VF;}^e~)ZyTnI$ylr%X3K3JU7qj;(?H9 zn~I(edb7cbFB2Mm9Hcw9x9+!>$NKGm?}gfD68D+=jc@iv!IV`Sz7mdYX!AwWAA`Oc z@qWO_E;D)#y7c+2R_EitZ_}c>GOGN_&)Kl*{2UD3Q?M0{reSu~mYNVw)xJ8Uui;c= zoe#ZE;7w(Nx@B&CH|z8h@4g3*-6)^aLwAD{bzkVc*Ly86Vq394e+`1&E4Y0P2PI@_z!zM8u>JB)%VL0 z_e=+31Fq*q&t7}&tHw7&SLG$Np65LG`sDYvwR-XE4KLg}vvvCM0dW}<)5rTCAAM@Z zOG(FHnX}Am*WeM~wn|Lbe-{6*x*tq8w(`F1erb5NQcYf)t-@dQw&wQ(I zVan_qk*4!qT-Q85Yut{kj75pHv!?8Hb+f)uka|*7Hp)15Gr#YSna^F={*(8v{yXDN zz5VLKP4A7iR$G%bW9}1cH@>&p@XX0Ax)|eH-Pq~Iw6ImT8mwwF)MdO=`pCw|zyD>+ zwsA3s*xDZ_y)kf3#DN~quKIevc24xo(F<}n9ebz!u&1BD|H-`1R`q%FbDg+u@Df)| zlLqG78@?Q=HlN(tpy1;U-xa-ZFmuz^D`!3TogegF(Y-dSzIbj%hegv4Ur4XHs?nD} zW*x1uZ|?eQNw@3oI6m^Rr=Odc9k`^|dcU9Jf~RlrJM+yFFXpKkeAxLdZPK0-Qv-iE zKQDw`uM>N99e61uzESTs$q~z%wR|me#zw8jGp`Q3>~TL=xBe+N zYt-9E#?5HBC1GFn&stAz;`8m?>zx(^HXQU~!}D`r_I$?a?G;J;cBb7HhxKh;WJ&Pr zxh+3o$cAs{u<^b6oY|T3ZtJhMfBMF|GwMuloObww2^~Y5ZMq%P;iJ({OledY&|2NC zz|DJ!-?h;LT+Uq7KX++Nky9VvEq7}+|M%9uYg{*JFHLJ#!@En{T3^1BvT}a)hM!;f z@sseSDKm3Ch7IfZobvYh@AuYy@t2Lh=aWy&UKydf8+T{;jAZ9r?*VPwi`( zYIoxbr^akMIIm{n=!~RhV?VpHWl{~7$WaAnuQXe^BxZ7I_S)|kwD9@lUzWgrpEPfH z^)qAhoEN6H7=BW%N^Rn&>rycI+D|)buDNvXw_zy-;aiilYcA}#v~Kd*cRm}qS`!y@ z?YL^-@$U0q9s1R@%%u6dR;4`lbkv&q9=^vWyE@05QVtw3<>&V^J>6=5mayoW{?^y0 zgZI>^ciI23bNQOCN5&nxyFx$jrM=VNua!>2 z@vYLE-D|&W@Q_LAw~sGlk8fGs{G0IW15Tb?UMuJEyRW_6ziYoZk0oc_UpNtWRc-vR z=77SMi+onM=pA1eYMlATs+>1YpP$!!_??@doV^;Ashzyu^mb~yLz)^L@`mk8|2paX z1pUkF8eBZMW_seC8vU0y+*90kz>Q~=9bDQ>xS8*De6zCI{53f#f&AlUwT{<2u&N(=wev4J-cD#90JI#5@y@dFQ1ziGrwfMD9(}9O> zhV+^xPVOD|^6r=4s{2%2&1wy<)Ze`FR<2jP_mwwZ(}+9H_Sv)W?gFte@Mvhr=2H$&szIWgecxb>aSJoV}HOV1B^{fm9;m2+R-IC0>d zn`>Hq)N#`jL*M~2^QyWML;-JMfi zlRZ9iUGnR!oX!Oy1-VA&7iKqlZ|AbaA)jZin!WLf?L(e4zB#n0i%J)Ka>#Gv`>5+3 zc=gkR`#+x1E;m!P_gJoO@mo9Bw@j_`+@3ozd!qI{eWdlX*7HlBI(sR2^O=QV&6C4+ zc4$6f@h=VMcFw=Bso{YmFVN?+9~^IZ9xhn85qBD(lWytIlSyL_-nZFx#P)^hag4jc@c9<+7h8Nat#H**r^y%Edq#E(=y&y+=jtH$kg2NeuWGlxGJk(Y2ce?!Y=+}1UY`*&M=>EQao+eO?e?BX; ziSO|H{bI}~r?>ic)j-#K_jCC+ZeqcCZ$9_jjjzf`BN{7p%#0MshG#r(nl*a3L@7BIH z4^fGfUM?6B(G7y|@Dsu=v zx}wKDd>{j#G(pgm6{<0*#(;2+E)<{Oi44W}7WDo?Mpk$if1M@VU$BhDm1Y#KGO{8l zE`s84r5S}Qe4_<(5~Q$Q$T7Gt#gwFJE622z)3lRg+Q~8P<(T$zOr#tW=`Y~ZSNc(C z5uN<;)f8QV6d_;vhw_&b>67(%!*+`ZPjK+#-6eLUAOZ7-6I$7a$liivT(R(ZCjH+IHXwa2lYxxMBYG`#~+x1Fg}Fm8iiw;3x1t1*J$-T|l)h z;0(|~%GnC3<@VBcFv4!M^J9S8Wd+^^)&YIcz8SzEhq-IBnlhdkNUxvZIRTy_(9;7vO~9iBJXVmO7~tUn9w8{$q2^Tk zzpo_jiyRxp+q70uYjTRzrnS_jwTg6vZJc$?L1CUN8l2d1k8>hgiCod(#EyH9%0>JO zlX4YOF5+L`;ocwooh|bzst+Y zcSf#!=X8W0RhG9)t;t2d@?737$mQ*lj_{*$EkdqzDc2&(wFtS=ADJu6wbtaqJG`tg zSL6zFO-ESLXwoVBgDk-QAkDWws8cRI=&qZ?ld-a4TH}}c3W*9_4s}L+o%Es<#d{`t z!HQnMqF2A^)o}Vu2Yu>;4(e#_`+(wTMZSW0!vhhR-&`g2PW&1{`K*Lj;s<3nQoj~W zRiOsEu>ri3(PLqF46Z1ax>xy-Qc-H^aBRJ@-8H%Ao{bAYN~v^pa&dN1Iyo1&W@lr@ zvK`c2(!mSDISt)i4ju)W89lrdQJ%p1qg;CLg1UqDprd3i*r2X(b5*)Xo}X%4dgRk( zDwfo9SO#*^Bignhy!0Bb&PqCX>9OUM9>X8!sd%0)u9U|Gkd8HsK?+* z4NYe|T9r_eYC?~Fvu&JWEJotDvkNl0Iw_rhRI)|i3wYjUI~LliAB$YXCwh#j;$_m~ z&TORehvLP!PxE6%^x{-DPe1XA@1IM4@B0=+#FdW>dqAh(#&FSzuSPqwNd+x&uWT&E z(9zV1l{@T`?pFO)mID!CWmA3&izWP-!_JXKOsYk4a%aV~xLy9??w-PL6u||TxB@u3 zXDb3ZDe`3aE$3)TKF;Bw&uNhRla8@C5#)mgYQ@D+yfQ~MXfnN>rGp(<@F(Koa(QuO zGT8#?b$NM`3-^GF^nf=jeE?iuo~pS2G6(LXn{h>l;CPVYo}IGrr#pSKz2AP(owaGX z{v=tA5jQa8hH$a(3ZR^G0TTK$fb?)J0PSMC0qS200n+ia0CiV40Iql-LTeZT&NMZm zc?YQr?KPrlKpf7Pu&@-IspF;nN8UK6~c=0oQ%i zmn;4sT)0njX<9m^GnGYWTIQoOQKdLKDmpht#i&gL{z?Q@fEEC#L1|k2Yfny!-%i6l zi7sOTZc}6wKv|*z3ez>U<;j%9xF-|nzvZTuv?+$}ApwymI`LS3r!%FO>C%~a+62IU z1HPpHdq1T_d8sboq`Q4Nv@Y{^ltZlhJIbLl{*H1`%zvyLqFr@VaiEF=RUD||Kotk7 zI8eobDh^a}po#-k9Qdm^AkRx_u0+qzX?{s_GHrd79(X zyq)Is^ems|?$~;ZuNjnXc=y9KZF;A<>V52M!G09H^)AhEX&x#7O#y#^?g!x99B2X1 zygd-0OLZ}#^tQSQ>khx?@Wlo{nJ1(}uGMIjLt4+l ziC9^3cyIEJr#%n75R-3Td%Fw|(#U&da!|mR^uS{OYG&NDa z0+D1hl`X#+(yB?xaSQ_Y>02mqIIH=$UZk%ENOvFoTOZ{U$H{IhTRu?@A!EVef-REt zg&Eo&O0uVKbY${6QB3JqKcv(T{s#U5{&yfUxwy`fb^Zv!BYxH-%WjRx%=FNjva#W{$E=m&Y-r^KTzjGgydA(1BWPXTXY>F-w%mm RH#)8xEh}Bs`G1B3{|~Js%8dX3 literal 0 HcmV?d00001 From b3e42ddfd752da7c9a9f212f7233d82fb0e08cb5 Mon Sep 17 00:00:00 2001 From: dgbillotte Date: Mon, 22 Nov 2010 17:24:11 -0700 Subject: [PATCH 2/2] job application by daniel@litfuze.com --- refactor-this/helper.rb | 63 ++++++----------------------------- refactor-this/helper_spec.rb | 58 +++++++++++++++----------------- refactor-this/photo.rb | 2 +- refactor-this/user_profile.rb | 5 +-- 4 files changed, 40 insertions(+), 88 deletions(-) diff --git a/refactor-this/helper.rb b/refactor-this/helper.rb index 6ef273b..83ff928 100644 --- a/refactor-this/helper.rb +++ b/refactor-this/helper.rb @@ -1,67 +1,24 @@ class Helper - def self.foo - "foo" - end - - def image_size(profile, non_rep_size) - if profile.user.rep? - '190x114' - else - non_rep_size - end - end - - def display_small_photo(profile, html = {}, options = {}) - display_photo(profile, image_size(profile, "32x32"), html, options) - end - - def display_medium_photo(profile, html = {}, options = {}) - display_photo(profile, image_size(profile, "48x48"), html, options) - end - - def display_large_photo(profile, html = {}, options = {}, link = true) - display_photo(profile, image_size(profile, "64x64"), html, options, link) - end - - def display_huge_photo(profile, html = {}, options = {}, link = true) - display_photo(profile, image_size(profile, "200x200"), html, options, link) - end def display_photo(profile, size, html = {}, options = {}, link = true) - return image_tag("wrench.png") unless profile # this should not happen + # NOTE: if this "should not happen" why is it here and why is there a unit test for it??? If the case is possible, it should be labeled as such. + return image_tag("wrench.png") unless profile # this should not happen show_default_image = !(options[:show_default] == false) - html.reverse_merge!(:class => 'thumbnail', :size => size, :title => "Link to #{profile.name}") - if profile && profile.user - if profile.user && profile.user.photo && File.exists?(profile.user.photo) - @user = profile.user - if link - return link_to(image_tag(url_for_file_column("user", "photo", size), html), profile_path(profile) ) - else - return image_tag(url_for_file_column("user", "photo", size), html) - end + if profile.has_valid_photo? + if link + return link_to() else - show_default_image ? default_photo(profile, size, {}, link) : '' + return image_tag() end end - show_default_image ? default_photo(profile, size, {}, link) : '' + show_default_image ? default_photo(profile, size, {}, link) : 'NO DEFAULT' end def default_photo(profile, size, html={}, link = true) - if link - if profile.user.rep? - link_to(image_tag("user190x119.jpg", html), profile_path(profile) ) - else - link_to(image_tag("user#{size}.jpg", html), profile_path(profile) ) - end - else - if profile.user.rep? - image_tag("user190x119.jpg", html) - else - image_tag("user#{size}.jpg", html) - end - end + link_to() end -end \ No newline at end of file +end + diff --git a/refactor-this/helper_spec.rb b/refactor-this/helper_spec.rb index b82e40e..643d5f9 100644 --- a/refactor-this/helper_spec.rb +++ b/refactor-this/helper_spec.rb @@ -1,102 +1,96 @@ -require 'rubygems' -require 'factory_girl' -require 'factories' -require 'spec' -require 'spec/autorun' -require 'redgreen' require 'user_profile' require 'helper' -require 'user' require 'photo' + describe "Helper" do before(:each) do @helper = Helper.new + @helper.stub!(:image_tag).and_return("wrench.png") end describe "display_photo" do it "should return the wrench if there is no profile" do @helper.display_photo(nil, "100x100", {}, {}, true).should == "wrench.png" end + describe "With a profile, user and photo requesting a link" do before(:each) do @profile = UserProfile.new @profile.name = "Clayton" - @user = User.new - @profile.user = @user - @photo = Photo.new - @user.photo = @photo + @photo = Photo.new + @profile.photo = @photo @profile.stub!(:has_valid_photo?).and_return(true) + + @helper.stub!(:link_to).and_return("this link") end it "should return a link" do @helper.display_photo(@profile, "100x100", {}, {}, true).should == "this link" end end - + describe "With a profile, user and photo not requesting a link" do before(:each) do @profile = UserProfile.new @profile.name = "Clayton" - @user = User.new - @profile.user = @user - @photo = Photo.new - @user.photo = @photo + @photo = Photo.new + @profile.photo = @photo @profile.stub!(:has_valid_photo?).and_return(true) + @helper.stub!(:image_tag).and_return("just image") end it "should just an image" do @helper.display_photo(@profile, "100x100", {}, {}, false).should == "just image" end end - + describe "Without a user, but requesting a link" do before(:each) do @profile = UserProfile.new @profile.name = "Clayton" + @helper.stub!(:link_to).and_return("default link 100x100") end it "return a default" do @helper.display_photo(@profile, "100x100", {}, {}, true).should == "default link 100x100" end end - + describe "When the user doesn't have a photo" do before(:each) do @profile = UserProfile.new @profile.name = "Clayton" - @user = User.new - @profile.user = @user @profile.stub!(:has_valid_photo?).and_return(false) end describe "With a rep user" do before(:each) do - @user.stub!(:rep?).and_return(true) + @profile.stub!(:rep?).and_return(true) + @helper.stub!(:link_to).and_return("default link 190x119") end it "return a default link" do @helper.display_photo(@profile, "100x100", {}, {}, true).should == "default link 190x119" end end - + describe "With a regular user" do before(:each) do - @user.stub!(:rep?).and_return(false) + @profile.stub!(:rep?).and_return(false) + @helper.stub!(:link_to).and_return("default link 100x100") end it "return a default link" do @helper.display_photo(@profile, "100x100", {}, {}, true).should == "default link 100x100" end end end - + describe "When the user doesn't have a photo and we don't want to display the default" do before(:each) do @profile = UserProfile.new @profile.name = "Clayton" - @user = User.new - @profile.user = @user @profile.stub!(:has_valid_photo?).and_return(false) end describe "With a rep user" do before(:each) do - @user.stub!(:rep?).and_return(true) + @profile.stub!(:rep?).and_return(true) end it "return a default link" do @helper.display_photo(@profile, "100x100", {}, {:show_default => false}, true).should == "NO DEFAULT" @@ -106,14 +100,14 @@ describe "With a regular user" do before(:each) do - @user.stub!(:rep?).and_return(false) + @profile.stub!(:rep?).and_return(false) end it "return a default link" do - @helper.display_photo(@profile, "100x100", {}, {}, true).should == "default link 100x100" + @helper.display_photo(@profile, "100x100", {}, {:show_default => false}, true).should == "NO DEFAULT" end end - end - + end + end -end \ No newline at end of file +end diff --git a/refactor-this/photo.rb b/refactor-this/photo.rb index 88ea4c5..87f7377 100644 --- a/refactor-this/photo.rb +++ b/refactor-this/photo.rb @@ -2,4 +2,4 @@ class Photo def to_s "photo.rb" end -end \ No newline at end of file +end diff --git a/refactor-this/user_profile.rb b/refactor-this/user_profile.rb index 36e6b87..4670064 100644 --- a/refactor-this/user_profile.rb +++ b/refactor-this/user_profile.rb @@ -2,7 +2,8 @@ class UserProfile attr_accessor :photo, :user, :name def has_valid_photo? - user && user.photo && File.exist?(user.photo) + #user && user.photo && File.exist?(user.photo) + photo && File.exist?(photo.to_s) end -end +end