Rack app to display username and current time #3

hemahg · 2019-12-18T05:24:24Z

No description provided.

hallelujah · 2019-12-19T10:11:18Z

RackApp/app.rb

+        if  req.path.match('/users/hema') && req.params["tz"] == "IST"
+            user = User.new("hema")
+            time = CountryTime.new("Kolkata")
+            view = View.new(user, time)
+            ['200', {"Content-Type" => "text/html"}, [view.render]]
+        elsif
+            req.path.match('/users/hery') && req.params["tz"] == "UTC"
+            user = User.new("Hery")
+            time = CountryTime.new("Eastern Time (US & Canada)")
+            view = View.new(user, time)
+            ['200', {"Content-Type" => "text/html"}, [view.render]]
+        elsif
+            req.path.match('/users/Cadu') && req.params["tz"] == "AST"
+            user = User.new("Cadu")
+            time = CountryTime.new("Asia/Kuwait")
+            view = View.new(user, time)
+            ['200', {"Content-Type" => "text/html"}, [view.render]]


Those are quite similar, can you refactor?

hallelujah · 2019-12-19T10:11:51Z

RackApp/app.rb

+class WebApp
+    def initialize
+
+    end
+end


This class does nothing sadly, can you remove or add logic there?

hallelujah · 2019-12-19T10:12:35Z

RackApp/app.rb

@@ -0,0 +1,85 @@
+require 'erb'
+require 'rack'
+require 'active_support/all'


We do not need to require all active_support. Always require only the necessary library if it is small

hallelujah · 2019-12-19T10:14:09Z

RackApp/app.rb

+require 'active_support/all'
+
+class View
+     attr_reader :person, :time


Your editor is configured to use 4 spaces as indentation, for various reason and better readability use 2 spaces

hallelujah · 2019-12-19T10:15:26Z

RackApp/template.html.erb

+<html>
+<head>
+<b>
+ persons nick name is <%= person.nickname%> and current time in the timezone is <%= time.timezone %>


Add space inside the ERB tags

Suggested change

persons nick name is <%= person.nickname%> and current time in the timezone is <%= time.timezone %>

persons nick name is <%= person.nickname %> and current time in the timezone is <%= time.timezone %>

hallelujah · 2019-12-19T10:17:09Z

RackApp/app.rb

+class Display
+    def self.call(env)
+        req = Rack::Request.new(env)
+        if  req.path.match('/users/hema') && req.params["tz"] == "IST"


So those are static values, can you make them dynamic?:

Display what you have in path as the nickname

Select the Timezone from the tz parameter as timezone

hallelujah · 2019-12-19T10:17:48Z

RackApp/app.rb

+require 'rack'
+require 'active_support/all'
+
+class View


This is more a controller than a view

hallelujah · 2019-12-23T12:01:15Z

RackApp/app.rb

@@ -0,0 +1,71 @@
+require "erb"


It would be nice to use rubocop to check for code styling ;)
You can make another commit for it

Example, rubocop will tell you to not forget about # frozen_string_literal: true

Some good reads why we use such tools: https://rubystyle.guide/

Also about the magic comment frozen_string_literal: true

https://www.mikeperham.com/2018/02/28/ruby-optimization-with-one-magic-comment/

This might go probably in Ruby 3.0 default https://bugs.ruby-lang.org/issues/11473 (still in discussion)

hallelujah · 2019-12-23T14:05:57Z

RackApp/app.rb

+    nickname = req.params["nickname"]
+    tz = req.params["tz"]
+    if req.path.match("/users") && nickname && tz
+      user = User.new("#{nickname}")


You can avoid String interpolation by using above nickane = params['nickname'].to_s and son on

macejmic · 2020-01-09T12:41:26Z

RackApp/app.rb

+end
+
+class CountryTime
+  attr_reader :zone, :timezone


attr_reader method creates a getter method within CountryTime class, getter class means/looks like this:

def timezone @timezone end

so by defining def timezone again, you're rewriting the created getter created by attr_reader :timezone

so either you use the created getter or create your own:

created getter by attr_reader :timezone

class CountryTime attr_reader :zone, :timezone def initialize(zone) @zone = zone Time.zone = zone @timezone = Time.zone.now end end

manually created "getter" by a method

class CountryTime attr_reader :zone def initialize(zone) @zone = zone end def timezone Time.zone = zone Time.zone.now end end

I would prefer the first way in this case, so timezone is being generated only once, it the second case you would generate the timezone in every method call.

macejmic · 2020-01-09T12:45:28Z

RackApp/app.rb

+    if req.path.match('/users') && nickname && tz && f
+      user = User.new(nickname.to_s)
+      time = CountryTime.new(tz.to_s)
+      view = MyController.new(user, time)


why time is being sent to MyController as a parameter and then never used?

also format could be another parameter and then you would call just view.render

macejmic · 2020-01-09T12:47:12Z

RackApp/app.rb

+    puts env.inspect
+    nickname = req.params['nickname']
+    tz = req.params['tz']
+    f = req.params['f']


f is not the ideal name for a variable.. the code should be readable as a book, so the variable names should make some sense.. timezone instead of tz, format instead of f

macejmic · 2020-01-09T12:48:26Z

RackApp/app.rb

+  end
+
+  def file(format)
+    File.read(File.join(__dir__, "template.#{format}.erb"))


you can take it a bit forward and check if the file actually exists.. if not, we can throw some meaningful error.

do you know how to do it? or should I help you for a bit? :)

hallelujah · 2020-01-09T17:19:43Z

RackApp/app.rb

+  def file(format)
+    File.read(File.join(__dir__, "template.#{format}.erb"))
+  end
+
+  def template(format)
+    ERB.new(file(format))
+  end


This looks like belonging to another class, name it View or Template

Something like that

class MyController # This next line could be extracted to a BaseController :) class_attribute :template_name, instance_writer: false template_name 'template' def template(format) Template.new(template_name, format) end end # rest of the code class Template def initialize(name, format) @erb = file(name, format) end def file(name, format) File.read(File.join(__dir__, 'templates', "#{name}.#{format}.erb")) end end

macejmic · 2020-02-04T09:09:00Z

RackApp/test/test.rb

+    personname = 'hema'
+    Time.zone = 'UTC'
+    puts @timezone = Time.zone.now
+    data = <<-HTML


Comparing the whole website is not the best idea. You would need to change this test every time you change a view HTML. Better to use https://apidock.com/ruby/Test/Unit/Assertions/assert_match and check for just small strings, something like:

assert_match "name is #{personname} and current", last_response.body

got the idea?

macejmic · 2020-02-04T09:09:30Z

RackApp/test/test.rb

+
+  def test_pagenotfound
+    get '/users/nickname'
+    assert_equal last_response.body, '<html><body><b><em>404 Page not found</em></b></body></html>'


the same as: 0c74d6c#r374548506

just check for 404 Page not found.

macejmic

these are integration tests, there are useful to check "response" behaviour as you did.. user should see this message and HTML should include this and that.. but along with integration tests would be good to add some unit tests too.

something like: https://en.wikibooks.org/wiki/Ruby_Programming/Unit_testing

hallelujah · 2020-02-17T12:59:06Z

RackApp/lib/myproject/app.rb

+    if File.exist?(file_path)
+      File.read(file_path)
+    else
+      'ERROR:The file doesnot exist'


raise an error instead ;)
Otherwise the result of the file will be displayed to the User
It is better to catch the error when it is really necessary and not assuming what it should do

hallelujah · 2020-02-17T12:59:37Z

RackApp/lib/myproject/app.rb

+  def call(env)
+    @app&.call(env)
+    req = Rack::Request.new(env)
+    puts env.inspect


You can remove this when you are done

hallelujah · 2020-02-17T13:04:14Z

RackApp/test/test.rb

+    assert_equal last_response.body, '<html><body><b><em>404 Page not found</em></b></body></html>'
+  end
+
+  def test_undefinedfileformat


use underscores to separate words

hallelujah · 2020-02-17T13:04:47Z

RackApp/lib/myproject/app.rb

+  end
+end
+
+class View


Normally I would like this class to be tested

hallelujah · 2020-02-17T13:06:08Z

RackApp/lib/myproject/app.rb

+  def initialize(zone)
+    @zone = zone
+    Time.zone = zone
+    @timezone = Time.zone.now


I do not get this, Time.zone.now will return an instance of Time.

Though the name timezone is in fact equal to zone

hallelujah · 2020-02-17T13:10:16Z

RackApp/lib/myproject/app.rb

+    timezone = req.params['timezone']
+    format = req.params['format']
+    nickname_match = req.path.match('/users/nickname/(\w+)')
+    nickname = nickname_match[1] if nickname_match


Suggested change

nickname = nickname_match[1] if nickname_match

nickname = $1

hallelujah · 2020-02-17T13:10:41Z

RackApp/lib/myproject/app.rb

+  end
+end
+
+class CountryTime


I would like to test this class

What if I send an invalid timezone?

hallelujah · 2020-02-17T13:11:31Z

RackApp/lib/myproject/app.rb

+      user = User.new(nickname)
+      time = CountryTime.new(timezone.to_s)
+      view = MyController.new(user, time)
+      ['200', { 'Content-Type' => response_format(format) }, [view.render_template(format)]]


What if I send an invalid format?

hallelujah · 2020-02-17T13:11:58Z

RackApp/lib/myproject/app.rb

+    format = req.params['format']
+    nickname_match = req.path.match('/users/nickname/(\w+)')
+    nickname = nickname_match[1] if nickname_match
+    if nickname && timezone && format


I think we should check about validity of the data more than the presence

hallelujah · 2020-02-17T13:12:42Z

RackApp/test/test.rb

+  end
+
+  def test_jsonoutput
+    puts Time.zone = 'UTC'


You are assigning Time.zone to 'UTC', is it intended?

hallelujah · 2020-02-17T13:13:07Z

RackApp/test/test.rb

+
+  def test_jsonoutput
+    puts Time.zone = 'UTC'
+    puts @timezone = Time.zone.now


You can remove the debugging lines when all is clean

hallelujah · 2020-02-17T13:14:25Z

RackApp/test/test.rb

+
+  def test_undefinedfileformat
+    get '/users/nickname/hema?timezone=UTC&format=xtml'
+    assert_equal last_response.body, 'ERROR:The file doesnot exist'


I would expect a status code 415 if the format does not exist

Rack app to display username and current time

306b9a5

hallelujah reviewed Dec 19, 2019

View reviewed changes

RackApp/app.rb Outdated

require 'rack'

require 'active_support/all'

class View

Copy link

hallelujah Dec 19, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is more a controller than a view

Incorporating review comments

23c5d66

hallelujah reviewed Dec 23, 2019

View reviewed changes

Updated code to display the output in xml and json format

5ba31a3

macejmic reviewed Jan 9, 2020

View reviewed changes

hallelujah reviewed Jan 9, 2020

View reviewed changes

hema and others added 2 commits January 22, 2020 14:01

Incorporating review comments

93e970e

Creating unit test for app

0c74d6c

macejmic reviewed Feb 4, 2020

View reviewed changes

hallelujah reviewed Feb 17, 2020

View reviewed changes

RackApp/lib/myproject/app.rb

end

end

class View

Copy link

hallelujah Feb 17, 2020 •

edited

Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Normally I would like this class to be tested

hallelujah reviewed Feb 17, 2020

View reviewed changes

RackApp/lib/myproject/app.rb

end

end

class CountryTime

Copy link

hallelujah Feb 17, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would like to test this class

What if I send an invalid timezone?

hallelujah reviewed Feb 17, 2020

View reviewed changes

RackApp/test/test.rb Outdated

end

def test_jsonoutput

puts Time.zone = 'UTC'

Copy link

hallelujah Feb 17, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You are assigning Time.zone to 'UTC', is it intended?

hallelujah reviewed Feb 17, 2020

View reviewed changes

Removing debugging line

147aa44

	persons nick name is <%= person.nickname%> and current time in the timezone is <%= time.timezone %>
	persons nick name is <%= person.nickname %> and current time in the timezone is <%= time.timezone %>

Rack app to display username and current time #3

Are you sure you want to change the base?

Rack app to display username and current time #3

Uh oh!

Conversation

hemahg commented Dec 18, 2019

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

hallelujah Jan 9, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

macejmic left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

hallelujah Feb 17, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

hallelujah Jan 9, 2020 •

edited

Loading

hallelujah Feb 17, 2020 •

edited

Loading