Help me decide what to do next?

[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)

[kikito]

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

(stuff)

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.

[Robin]

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

(stuff)

You forgot the self arguments, man.

[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

[kikito]

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.

Code: Select all

(stuff)

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.

[thelinx]

You forgot the self arguments, man.

That's a python class definition, where functions with self arguments are implied. Right?

[kikito]

You forgot the self arguments, man.

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.

[lesslucid]

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.

[nevon]

You forgot the self arguments, man.

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).

[BlackBulletIV]

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*