Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

GUI uses nearly the same name for four different change set variables #2276

Open
HebaruSan opened this issue Feb 6, 2018 · 5 comments
Open
Labels
GUI Issues affecting the interactive GUI

Comments

@HebaruSan
Copy link
Member

HebaruSan commented Feb 6, 2018

Problem

These are all members of CKAN.Main in GUI:

private IEnumerable<ModChange> currentChangeSet;

private IEnumerable<ModChange> ChangeSet

private List<ModChange> changeSet;

public void UpdateChangesDialog(List<ModChange> changeset, BackgroundWorker installWorker)

All four names are available for use inside of CKAN.Main.UpdateChangesDialog. To call this confusing would be an understatement. In terms of meaning, these names are indistinguishable, and at the syntactic level 3 of them vary only in their capitalization. If a programmer needs to read or write from the current change set, how likely is he to be able to figure out which variable does what? If a programmer is trying to investigate a problem with change sets, how easy will it be to discern the purpose of code using these variables?

Suggestions

The three member variables should be consolidated into one. It's reasonable to have one private member variable tracking change sets, but two or three is asking for trouble.

When used as a parameter, the name should have a prefix describing how it's different from the member variable; for example it could be called newChangeSet. There would be much less risk of accidentally typing newChangeSet when you meant to use the main changeSet variable, or vice versa.

@politas
Copy link
Member

politas commented Feb 13, 2018

I have always found this "feature" of the codebase a bit odd. My programming background strongly favours clearly described variable names. I think there are many other cases in our code where similar things happen, including some where the only difference is that one variable uses the "wrong" naming scheme.

@HebaruSan
Copy link
Member Author

This line is setting the wrong one:

CKAN/GUI/MainInstall.cs

Lines 73 to 76 in ddf9ac3

finally
{
changeSet = null;
}

@HebaruSan
Copy link
Member Author

HebaruSan commented Feb 1, 2020

We eliminated Main.changeSet in #2966 with the migration of the change set tab's logic into a UserControl, but we replaced it with a Main.Changeset control. So we still have several objects with nearly the same name, but now one of them is a UserControl derived class rather than List<ModChange>.

@politas
Copy link
Member

politas commented Feb 6, 2020

So why isn't that called Main.ChangesetControl or even ChangesetUserControl in order to make what it is a bit easier to understand?

@HebaruSan
Copy link
Member Author

When it was created, the existing custom controls in Main.Designer.cs were named after their types, which didn't end in Control or UserControl except for MainTabControl:

        public CKAN.MainModListGUI ModList;
        private CKAN.MainModInfo ModInfo;
        private CKAN.MainTabControl MainTabControl;
        private CKAN.HintTextBox FilterByAuthorTextBox;

I was trying to follow precedent rather than revamp it on this point.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
GUI Issues affecting the interactive GUI
Projects
None yet
Development

No branches or pull requests

3 participants