Code review comment for lp:~al-maisan/landscape-client/obsolete-results-v

Hello Free,

thank you very much for these comments, I really appreciate it.
I have taken care of all of them, please see the attached incremental
diff.

On 04/30/2010 11:14 AM, Free Ekanayaka wrote:
> [9]
>
> + message_context_store,
>
> Please name this parameter "exchange_store", so it matches the name of the expected type (ExchangeStore) and it is shorter. Also, now that we have it, the ExchangeStore could be used to store also other types of information beside MessageContext's, so it makes sense to use a generic name for it.
>
> + self._store = message_context_store
>
> Similarly please name this attribute "self._exchange_store", to make it clear which store is referring to (there is another attribute called "self._message_store", so the naming will be consistent).
>
> [10]
>
> + else:
> + if "timestamp" not in message:
> + message["timestamp"] = int(self._reactor.time())
> + message_id = self._message_store.add(message)
> + if urgent:
> + self.schedule_exchange(urgent=True)
> + return message_id
>
> I think you can save this else/indentation level and simply write:
>
> + if "timestamp" not in message:
> + message["timestamp"] = int(self._reactor.time())
> + message_id = self._message_store.add(message)
> + if urgent:
> + self.schedule_exchange(urgent=True)
> + return message_id
>
> I personally like the pattern when you check a series of early-return conditions, before reaching the meat of a function/method, like
>
> if this:
> return something
> if that:
> return something_else
>
> do_the_actual_thing
>
> [11]
>
> + exchange_store = ExchangeStore(
> + os.path.join(config.data_path, "exchange.database"))
>
> Just to avoid hard-coding this value you could add a property to l.broker.config.BrokerConfiguration, like:
>
> @property
> def exchange_store_path(self):
> return os.path.join(self.data_path, "exchange.database"))
>
> and use it as
>
> + exchange_store = ExchangeStore(self.config.exchange_store_path)
>
> [12]
>
> + exchange_store = ExchangeStore(
> + os.path.join(test_case.config.data_path, "exchange.database"))
>
> Please add this object to the test_case attributes, so it can be used in tests:
>
> + test_case.exchange_store = ExchangeStore(
> + os.path.join(test_case.config.data_path, "exchange.database"))
>
> after that you can avoid creating it explicitly in all the tests.
>
> [13]
>
> + self.exchanger._store.get_message_context(message['operation-id']))
>
> Once you have self.exchange_store as test attribute (see [12]) you should use it instead of accessing this private attribute.
>
> Thanks!

Best regards

--
Muharem Hrnjadovic <email address hidden>
Public key id : B2BBFCFC
Key fingerprint : A5A3 CC67 2B87 D641 103F 5602 219F 6B60 B2BB FCFC