Pull trading code into it's own library.

[cwhelchel]

See issue #20. I'd also say issue #35 as well as I did add some comments where there weren't any.

This is a pretty big pull so be prepared for a headache. I've probably broke stuff for many people.

I've tested the functionality of the new TradesSession class and it appears to work for adding items, removing them, accepting trades, and canceling trades. I've also merged in the master branch to minimize some conflicts.

Let me know what I've missed as I'm sure there is something.

[Philipp15b]

'Fetches' 👅

[Philipp15b]

@cwhelchel I do not agree with your changs in the Trade class at all.

I don't get why you set so many properties private that may be needed by UserHandlers, they should only be readonly from outside the class.

I also don't get why you split Trade into two Trade and TradeSession classes. That makes no sense to me; a Trade is already a session. If you just wanted to split the files, why not create a partial class that spans over multiple files if necessary?

And I also don't like how you integrated logging into the trade, in my understanding, moving trading to a library means removing all dependencies from the trading part to the bot itself. The bot should have to register callbacks which then log.

[cwhelchel]

@Philipp15b Let me try to change your mind.

The Trade class was responsible for a couple of things:

• Building URLs that interact with the steam trading website
• Managing the inventories and the offered items for both persons
• Firing events based around the trade itself

I tried my best to split off the URL building Steam Trade interaction as best I could. This would allow for testing of the Trade class without access a real trade (if it were tested). And I also think it's a good place to separate responsibilities. URL building and fetching has little to do with an actual Trade object, it's merely a "data provider" if you will.

There are probably more things that could be added to the TradeSession class to make the split cleaner. This is probably what you are getting at in your last paragraph about the Logging.

As far as making various data private, I feel that needs no arguing. Data should be private and exposed as public properties. Public data clutters up your public interface, which is way more important than access to the data itself. Granted, I made all data private that was not used externally, and some of these should be exposed via properites. I was not sure what would and would not be used, so this is something that should be exposed in the future.

[Philipp15b]

@cwhelchel I'm still not convinced.

I still do not see a reason why you created a TradeSession class; it just contains almost boilerplate code which that doesn't even contain much business logic. There is nothing special to split it up for testing, the URL building part isn't even in there, it's part of SteamWeb. You use classes of TradeSession in Trade... It all makes things more complicated.

Regarding logging, you're correct. I'd love to see some cleaner splitting even if that means some more code.

What you said about data encapsulation is also something I do not understand. When designing the API, I deliberately made those properties public. For data that is passed into the trade, I do not see a reason to hide them from the user. What I didn't do though was making those fields externally read-only, which is something that should definitely be added. The following properties should be publicy available:

• Ready state properties; you will use them in UserHandlers to check if both users are ready. Why the heck should I copy that data in my UserHandler?
• The Steam IDs; this is something that makes up the identity of a Trade.
• Trade time information, could be used for logging.

[Philipp15b]

By the way, squashing your commits would also be nice...

[cwhelchel]

@Philipp15b Regarding the TradeSession: It's going to be boilerplate code no matter where you put it. However, there are several things that can be done in incorporate more functionality (and make it less boilerplate). These are definitely on my TODO list:

• Error handling the JSON return status from the web fetches. This is important and it wasn't done before. The lack of this made it harder to write this branch.
• Some internal events for interaction between Trade and TradeSession (reduce some complexity of Trade.Poll())
• Wrapping TradeSession in an interface would reduce the coupling between Trade and TradeSession. Would make it easier in case the web stuff changes or we want to write a mock object class.
• Rename TradeSession to something more descriptive (Session was bad wording on my part) :-)

If the code was still in Trade, the above changes would bloat that class even further. In the end this will make Trade easier work with in the UserHandlers especially as features get added to Trade.

You're right about those other fields. I'll work on a commit that has them exposed as read-only properties.

[Philipp15b]

@cwhelchel You understood me wrong. I have absolutely no problem with the boilerplate code here. Now with code folding I have no problem with a 1000 lines class of things that belong together. I'd love to see both classes merged again.

The only reason of yours I can understand is testing, but that reminds me of Java's class hell. I see no reason to make it an interface. Instead of replacing the TradeSession with an interface and using mocks and stuff, why not just use compiler flags in the Fetch method. Then, when testing, it would redirect to a mock, which would then contain a dictionary of mocked results. This way, you could test all methods perfectly fine while maintaing order.

As said before, if you have problems with too long files, just use partial classes.

[cwhelchel]

@Philipp15b I think we're splitting hairs a bit. Either a separate class or a partial class will work.

Personally I don't think a user needs access to the web methods and should only work with the Trade class. The Trade class also should do some of the error checking (and logging) as someone writing a User handler may not check returns or log anything.

I'll change it to a partial class if anyone else speaks up i.e. its not a decision I think I should make.

[Philipp15b]

@cwhelchel Many thanks for this.

I don't understand what you mean by

I don't think a user needs access to the web methods and should only work with the Trade class

A user should just use AddItem, RemoveItem, etc. It should just appear as local data.

Error checking and logging should be kept out of the Trade. This is something that can be easily added by events and added into SteamBot. This is also why I'd move the Log class out of SteamTrading into SteamBot. The trading library should be only oriented on providing a simple and secure API to Steam's Trading.

[cwhelchel]

@Philipp15b I think we are thinking the same thing. I was just expressing it directly that a user should only call AddItem and RemoveItem and these methods will do things like mutate local data and call the methods to do the web fetching. These should also, IMO, log errors that a user may or may not be aware of.

I think logging is part of making a secure and easy to use API. But that discussion is tangential to this pull request. I moved the Log class into the trade library for pragmatic reasons not ideological reasons.

[teliosdev]

Why not use exceptions within the Trade class, and move the Log class out of SteamTrading?

[Philipp15b]

@redjazz96 👍

[cwhelchel]

@redjazz96 Agreed. That's a better design. Will change.

[teliosdev]

If I remember right, this should increment the SteamBot version...

[teliosdev]

Why did you do this? The whole point of it was to allow AddItemByDefindex to add two items with the same Defindex without moving them instead.

[cwhelchel]

Do you mean moving them in the trade slots? I think I removed this originally because I thought it was doing the same thing twice. Add to _Offered and copy that to MyOfferedItems.

I was unaware that it was required to get that method to work.

[teliosdev]

When the Poll updates every 800ms, OnTradeAddItem is called when an item is added. If an item is added in the function, MyOfferedItems isn't updated until the next Poll.

What I did was I added a line to AddItem (and RemoveItem) that added the item that the user added to MyOfferedItems; the lines that you commented out updated MyOfferedItems to the list that steam has, so that it stays in sync with steam (as the request in AddItem or RemoveItem could have failed). _OfferedItemsFromSteam always had the list from steam, so every item in there is pretty much guaranteed to be offered in the trade (unlike MyOfferedItems, for reasons stated above).

[cwhelchel]

I copied the semver stuff in the AssemblyInfo file in the SteamTrade as well... I'm not familiar with this versioning so please answer:

In SteamBot/AssemblyInfo.cs:

this:
[assembly: AssemblyVersion("0.0.1.*")]

will become this:
[assembly: AssemblyVersion("0.0.2.*")]

And do you think this should be the same for the SteamTrade assembly?

[teliosdev]

@cwhelchel I'd think it'd jump up to 0.1.0.* for SteamBot, and stay at 0.0.1.* for SteamTrade. I don't think we're ready for a public API release yet.

[teliosdev]

Just a little note for merging: fixes #66, #20, #35

[cwhelchel]

How does the 183ca6c look for merging? Anyone think there are more changes that should to be done?

This should close out issue #66.

[Philipp15b]

I don't get why there are Cmd versions of each method that don't throw exception and instead return booleans. I still do not support this weird kind of task splitting...

[cwhelchel]

They are more or less just helper functions that make the other methods (like RemoveItem or AddItem) that the user will actually call easier read and change. Perhaps throwing exceptions in the public methods is a little hard to understand. I can add exceptions specific to the functions that interact with the web and throw them in the *WebCmd functions.

The public methods AddItem et. al. do other things besides to fetching and posting urls. Refactoring this into a separate method is not "weird". I fail to see why you think it is.

[Philipp15b]

I just think it makes the code more complicated. There is not much code that is there besides network stuff. You search longer to find very simple functionality. I simply don't get why you seperate these things. The functionality in the Cmd methods is used only once, in the public methods. So why seperate that? The code may seem shorter, but for the trade-off of more complicated code.

[cwhelchel]

You search longer to find very simple functionality

IDE's are glorified file systems managers and have great search capabilities. Arguing that the 5ms it takes to find a method (press F12 on the method name) makes a code base more complicated, is a bit of a stretch. Especially one that is already quite small (just open the two files that have Trade in the name). And this is not to mention the fact that this is all internal crap that most people aren't going to care about (they probably just want to write UserHandlers and use a simple Trade class).

Of course, we could put every single piece of code in one gigantic file and make life easier for everyone? Maybe even all in one method... that would work out well.

[cwhelchel]

I'm sorry, I realize I sound like a jerk in that last comment.

@Philipp15b I know your just trying to do what you think is best. So am I.

[Philipp15b]

As long as there are no additional opinions on this, I think we should merge this now as I don't see an agreement, so we'll worry about that later.

/cc @redjazz96, @FunkyLoveCow