Page 3 of 4
Re: Help me decide what to do next?
Posted: Thu Jul 21, 2011 10:20 am
by nevon
On a somewhat related note, what are your thoughts on method arguments vs. using instance variables? To clarify, which of these would you prefer:
Code: Select all
Class DerpFactory:
def __init__(goal):
self.goal = goal
def produceDerps(goal):
print ("Derp!\n"*goal)
Code: Select all
Class DerpFactory:
def __init__(goal):
self.goal = goal
def produceDerps():
print ("Derp!\n"*self.goal)
Re: Help me decide what to do next?
Posted: Thu Jul 21, 2011 10:46 am
by kikito
nevon wrote:On a somewhat related note, what are your thoughts on method arguments vs. using instance variables? To clarify, which of these would you prefer:
It depends.
Classes, like functions, should be small, and do one and only one thing, whenever possible.
This means that you can't usually have lots of instance variables to begin with. On the other hand, you want to minimize the number of parameters you pass through.
Well, in my case, the decision of instance variable vs parameter depends on how many times is it used in a class. If a param is used in more than one method, I consider very seriously transforming it into an instance variable. If this contradicts other methods, or if I end up with lots of instance variables, then I consider splitting up my class into smaller ones (probably grouping the methods by the instance variables/parameters they use).
Similarly, if an instance variable is only used in one method, then I think about making it a parameter, or I try to get by without that parameter.
It's more usual for me to transform a parameter into an instance variable than the other way around, but that has happened too.
Re: Help me decide what to do next?
Posted: Thu Jul 21, 2011 10:48 am
by Robin
nevon wrote:On a somewhat related note, what are your thoughts on method arguments vs. using instance variables? To clarify, which of these would you prefer:
You forgot the
self arguments, man.
![Wink ;)](./images/smilies/ms-wink.png)
Re: Help me decide what to do next?
Posted: Thu Jul 21, 2011 12:24 pm
by kraftman
Nice explanation, thanks
I think I was thinking of functions in the wrong way. In my mind one function that does all of the things it needs to do is less bulky then splitting it into loads of different functions.
I learnt Lua through WoW so some of the first example code i saw had functions like this:
Code: Select all
function TradeSkillFrame_SetSelection(id)
local skillName, skillType, numAvailable, isExpanded, altVerb = GetTradeSkillInfo(id);
local creatable = 1;
if ( not skillName ) then
creatable = nil;
end
TradeSkillHighlightFrame:Show();
TradeSkillGuildFrame.queriedSkill = nil; -- always cancel any pending queries
TradeSkillGuildFrame:Hide();
if ( skillType == "header" ) then
TradeSkillHighlightFrame:Hide();
if ( isExpanded ) then
CollapseTradeSkillSubClass(id);
else
ExpandTradeSkillSubClass(id);
end
return;
end
TradeSkillFrame.selectedSkill = id;
SelectTradeSkill(id);
if ( GetTradeSkillSelectionIndex() > GetNumTradeSkills() ) then
return;
end
TradeSkillSkillName:SetText(skillName);
local cooldown, isDayCooldown = GetTradeSkillCooldown(id);
if ( not cooldown ) then
TradeSkillSkillCooldown:SetText("");
elseif ( not isDayCooldown ) then
TradeSkillSkillCooldown:SetText(COOLDOWN_REMAINING.." "..SecondsToTime(cooldown));
elseif ( cooldown > 60 * 60 * 24 ) then --Cooldown is greater than 1 day.
TradeSkillSkillCooldown:SetText(COOLDOWN_REMAINING.." "..SecondsToTime(cooldown, true, false, 1, true));
else
TradeSkillSkillCooldown:SetText(COOLDOWN_EXPIRES_AT_MIDNIGHT);
end
TradeSkillSkillIcon:SetNormalTexture(GetTradeSkillIcon(id));
local minMade,maxMade = GetTradeSkillNumMade(id);
if ( maxMade > 1 ) then
if ( minMade == maxMade ) then
TradeSkillSkillIconCount:SetText(minMade);
else
TradeSkillSkillIconCount:SetText(minMade.."-"..maxMade);
end
if ( TradeSkillSkillIconCount:GetWidth() > 39 ) then
TradeSkillSkillIconCount:SetText("~"..floor((minMade + maxMade)/2));
end
else
TradeSkillSkillIconCount:SetText("");
end
-- Reagents
local numReagents = GetTradeSkillNumReagents(id);
if(numReagents > 0) then
TradeSkillReagentLabel:Show();
else
TradeSkillReagentLabel:Hide();
end
for i=1, numReagents, 1 do
local reagentName, reagentTexture, reagentCount, playerReagentCount = GetTradeSkillReagentInfo(id, i);
local reagent = _G["TradeSkillReagent"..i]
local name = _G["TradeSkillReagent"..i.."Name"];
local count = _G["TradeSkillReagent"..i.."Count"];
if ( not reagentName or not reagentTexture ) then
reagent:Hide();
else
reagent:Show();
SetItemButtonTexture(reagent, reagentTexture);
name:SetText(reagentName);
-- Grayout items
if ( playerReagentCount < reagentCount ) then
SetItemButtonTextureVertexColor(reagent, 0.5, 0.5, 0.5);
name:SetTextColor(GRAY_FONT_COLOR.r, GRAY_FONT_COLOR.g, GRAY_FONT_COLOR.b);
creatable = nil;
else
SetItemButtonTextureVertexColor(reagent, 1.0, 1.0, 1.0);
name:SetTextColor(HIGHLIGHT_FONT_COLOR.r, HIGHLIGHT_FONT_COLOR.g, HIGHLIGHT_FONT_COLOR.b);
end
if ( playerReagentCount >= 100 ) then
playerReagentCount = "*";
end
count:SetText(playerReagentCount.." /"..reagentCount);
end
end
-- Place reagent label
local reagentToAnchorTo = numReagents;
if ( (numReagents > 0) and (mod(numReagents, 2) == 0) ) then
reagentToAnchorTo = reagentToAnchorTo - 1;
end
for i=numReagents + 1, MAX_TRADE_SKILL_REAGENTS, 1 do
_G["TradeSkillReagent"..i]:Hide();
end
local spellFocus = BuildColoredListString(GetTradeSkillTools(id));
if ( spellFocus ) then
TradeSkillRequirementLabel:Show();
TradeSkillRequirementText:SetText(spellFocus);
else
TradeSkillRequirementLabel:Hide();
TradeSkillRequirementText:SetText("");
end
if ( creatable ) then
TradeSkillCreateButton:Enable();
TradeSkillCreateAllButton:Enable();
else
TradeSkillCreateButton:Disable();
TradeSkillCreateAllButton:Disable();
end
if ( GetTradeSkillDescription(id) ) then
TradeSkillDescription:SetText(GetTradeSkillDescription(id))
TradeSkillReagentLabel:SetPoint("TOPLEFT", "TradeSkillDescription", "BOTTOMLEFT", 0, -10);
else
TradeSkillDescription:SetText(" ");
TradeSkillReagentLabel:SetPoint("TOPLEFT", "TradeSkillDescription", "TOPLEFT", 0, 0);
end
-- Reset the number of items to be created
TradeSkillInputBox:SetNumber(GetTradeskillRepeatCount());
local skillLineName, skillLineRank, skillLineMaxRank, skillLineModifier = GetTradeSkillLine();
local color;
--Hide inapplicable buttons if we are inspecting. Otherwise show them
if ( IsTradeSkillGuild() ) then
-- highlight color
color = TradeSkillTypeColor["easy"];
-- title
TradeSkillFrameTitleText:SetFormattedText(GUILD_TRADE_SKILL_TITLE, skillLineName);
-- bottom bar
TradeSkillCreateButton:Hide();
TradeSkillCreateAllButton:Hide();
TradeSkillDecrementButton:Hide();
TradeSkillInputBox:Hide();
TradeSkillIncrementButton:Hide();
TradeSkillLinkButton:Hide();
TradeSkillViewGuildCraftersButton:Show();
if ( GetTradeSkillSelectionIndex() > 0 ) then
TradeSkillViewGuildCraftersButton:Enable();
else
TradeSkillViewGuildCraftersButton:Disable();
end
-- status bar
TradeSkillRankFrame:Hide();
else
-- highlight color
color = TradeSkillTypeColor[skillType];
-- title
TradeSkillFrameTitleText:SetFormattedText(TRADE_SKILL_TITLE, skillLineName);
-- bottom bar
TradeSkillViewGuildCraftersButton:Hide();
-- status bar
TradeSkillRankFrame:SetStatusBarColor(0.0, 0.0, 1.0, 0.5);
TradeSkillRankFrameBackground:SetVertexColor(0.0, 0.0, 0.75, 0.5);
TradeSkillRankFrame:SetMinMaxValues(0, skillLineMaxRank);
TradeSkillRankFrame:SetValue(skillLineRank);
if ( skillLineModifier > 0 ) then
TradeSkillRankFrameSkillRank:SetFormattedText(TRADESKILL_RANK_WITH_MODIFIER, skillLineRank, skillLineModifier, skillLineMaxRank);
else
TradeSkillRankFrameSkillRank:SetFormattedText(TRADESKILL_RANK, skillLineRank, skillLineMaxRank);
end
if IsTrialAccount() then
local _, _, profCap = GetRestrictedAccountData();
if skillLineRank >= profCap then
local text = TradeSkillRankFrameSkillRank:GetText();
text = text.." "..RED_FONT_COLOR_CODE..TRIAL_CAPPED
TradeSkillRankFrameSkillRank:SetText(text);
end
end
TradeSkillRankFrame:Show();
local linked = IsTradeSkillLinked();
if ( linked ) then
TradeSkillCreateButton:Hide();
TradeSkillCreateAllButton:Hide();
TradeSkillDecrementButton:Hide();
TradeSkillInputBox:Hide();
TradeSkillIncrementButton:Hide();
TradeSkillLinkButton:Hide();
else
--Change button names and show/hide them depending on if this tradeskill creates an item or casts something
if ( not altVerb ) then
--Its an item with 'Create'
TradeSkillCreateAllButton:Show();
TradeSkillDecrementButton:Show();
TradeSkillInputBox:Show();
TradeSkillIncrementButton:Show();
else
--Its something else
TradeSkillCreateAllButton:Hide();
TradeSkillDecrementButton:Hide();
TradeSkillInputBox:Hide();
TradeSkillIncrementButton:Hide();
--TradeSkillFrameBottomLeftTexture:SetTexture([[Interface\ClassTrainerFrame\UI-ClassTrainer-BotLeft]]);
--TradeSkillFrameBottomRightTexture:SetTexture([[Interface\ClassTrainerFrame\UI-ClassTrainer-BotRight]]);
end
if ( GetTradeSkillListLink() ) then
TradeSkillLinkButton:Show();
else
TradeSkillLinkButton:Hide();
end
TradeSkillCreateButton:SetText(altVerb or CREATE);
TradeSkillCreateButton:Show();
end
end
TradeSkillFrame_SetLinkName();
if ( color ) then
TradeSkillHighlight:SetVertexColor(color.r, color.g, color.b);
end
end
Re: Help me decide what to do next?
Posted: Thu Jul 21, 2011 12:47 pm
by kikito
kraftman wrote:Nice explanation, thanks
I think I was thinking of functions in the wrong way. In my mind one function that does all of the things it needs to do is less bulky then splitting it into loads of different functions.
I mean no disrespect, but that was ... bad.
"Bulky" is the linchpin here. True, separating into more functions yields usually more lines of code. More lines of code increase the complexity. But if those lines of code look like English and less than just symbols, the complexity is reduced.
The complexity you "gain" by adding lines of code is much smaller than the one you gain by keeping everything in a single, big function.
Re: Help me decide what to do next?
Posted: Thu Jul 21, 2011 2:48 pm
by thelinx
Robin wrote:
You forgot the
self arguments, man.
![Wink ;)](./images/smilies/ms-wink.png)
That's a python class definition, where functions with self arguments are implied.
Right?
Re: Help me decide what to do next?
Posted: Thu Jul 21, 2011 2:51 pm
by kikito
thelinx wrote:Robin wrote:
You forgot the
self arguments, man.
![Wink ;)](./images/smilies/ms-wink.png)
That's a python class definition, where functions with self arguments are implied.
Right?
Python's self is explicit. But I got the point anyway.
Re: Help me decide what to do next?
Posted: Thu Jul 21, 2011 3:08 pm
by lesslucid
kikito wrote:
The problem lesslucid has is that his functions look a lot like that one (or are even more complex).
Is it really that bad, I thought? Then I check... I have an updateBall function with almost 100 lines!
...this gives me an idea of a gentle way back into coding; I will try to move all of that functionality out into smaller functions and then call all those functions from within updateBall...
Thanks! I really appreciate all the helpful discussion and criticism.
![Smile :)](./images/smilies/ms-smile.png)
Re: Help me decide what to do next?
Posted: Thu Jul 21, 2011 3:23 pm
by nevon
thelinx wrote:Robin wrote:
You forgot the
self arguments, man.
![Wink ;)](./images/smilies/ms-wink.png)
That's a python class definition, where functions with self arguments are implied.
Right?
No, that was just a mess-up on my part. I haven't really written any Python in ages, but I still find the syntax to be extremely readable (and hence good for examples like this).
Re: Help me decide what to do next?
Posted: Fri Jul 22, 2011 3:37 am
by BlackBulletIV
kraftman wrote:In my mind one function that does all of the things it needs to do is less bulky then splitting it into loads of different functions.
*Huge sigh*