>
>
> @@ -25,8 +26,17 @@ class ImageBuilderConsole < Qmf::ConsoleHandler
>
> def initialize(args={})
> @settings = Qmf::ConnectionSettings.new
> - args.include?(:host) ? @settings.host = args[:host] :
> - args.include?(:port) ? @settings.host = args[:port] :
> + args.include?(:host) ? @settings.host = args[:host] : nil
> + args.include?(:port) ? @settings.host = args[:port] : nil
>
I believe this is error, should it be like:
@settings.host = args.include?(:host) ? args[:host] : nil
or even better:
@settings.host = args[:host]
It's because if there's no key for Hash is always returned. All the others
could b e rewritten the same way...
> def find_build(build_id)
> + @logger.info "Entering find_build method..."
>
I see that you're accessing instance variables all over the place. I believe
that it should be encapsulated if possible in to accessor (or reader). The
benefit is that in future you want to extend, change, logger you just
rewrite the method no big refactoring needed.
> ab = @qmfc.objects(Qmf::Query.new(:class => "ActiveBuild"))
> ab.each do |build|
> return build if build.object_id.to_s.eql?(build_id)
>
>
This is simple map or select no need for each here:
ab.map { |build| build.object_id.to_s == build_id }
> def check_status(active_build)
> - active_build.update
> - active_build.status
> + begin
> + active_build.update
> + active_build.status
> + rescue => e
> + @logger.error "#{e.backtrace.shift}: #{e.message}"
> + e.backtrace.each do |step|
> + @logger.error "\tfrom #{step}"
> + end
> + @logger.info "Attempting to get a new ActiveBuild object with: " +
> active_build.object_id.to_s
> + return "failed" if check_for_failed
> + ab = find_build(active_build.object_id)
> + ab.size > 0 ? check_for_failed(ab.first) : "failed"
> + end
> end
>
No need for begin... rescue could be just:
def method(xx)
rescue => e
end
>
>
> + unless @connection.connected?
> + @broker = @qmfc.add_connection(@connection)
> + sleep 0.5
> + @broker.wait_for_stable
> + retrieve_factory_instance
> + end
+ end
>
Would be nice to have @connection.connected? encapsulated inside method.
Refactoring would be easier in future. Also nested 'unless' statement can be
avoided. So you have just one indention level. Like this:
return if @connectoin.connected?
> +
> + def retrieve_factory_instance
> + @logger.info "Trying to get an instance of ImageBuilder..."
> + unless @connection.connected?
> + establish_connection
> + end
> + attempts = 0
> + loop do
> + attempts += 1
> + @ib = @qmfc.object(Qmf::Query.new(:class => "ImageBuilder"))
> + #[email protected] "Retrieved: " + @ib.inspect
> + break if @ib
> + @logger.info "No ImageBuilder object found, trying again in
> #...@delay} seconds..."
> + sleep @delay
> + if attempts > @retry_limit
> + @logger.info "Giving up after #...@retry_limit} attempts"
> + break
> + end
> + end
> + return
> + end
>
I believe that the 'return' statement on the end of the method is not
needed. Also using loop is considered dangerous because if you're 'break'
logic is broken it will loop infinitely. Using while or for cycle with
predefined maximum is safer.
> +
> + def check_for_failed
> + true unless @qmfc.object(Qmf::Query.new(:class => "ImageBuilder")) &&
> @connection.connected?
> + end
>
No need for unless if there's logic expression itself. Maybe better this
way:
def check_for_failed
! (@qmfc.object(Qmf::Query.new(:class => "ImageBuilder")) &&
@connection.connected?)
end
+
> + def valid_factory?
> + begin
> + @ib.update
> + return true
> + rescue => e
> + return false
> end
> end
> end
Maybe easier to write:
def valid_factory?
@ib.update && true rescue false
end
Maybe it would make sense to look insde @ib.update if it returns true false
than just:
def valid_factory?
@ib.update rescue false
end
Also if update can raise exception(s) it should be named with bang like:
update!
Hope the feedback will be appreciated.
--Ladislav
_______________________________________________
deltacloud-devel mailing list
[email protected]
https://fedorahosted.org/mailman/listinfo/deltacloud-devel
