Some tidying up is needed

hol430 commented 4 years ago

As part of my work on refactoring the Apsim.Get API, I've come across some strange code which is either inefficient, hard to read, doesn't meet our style guidelines, or some combination thereof. Specifically:

APSIM.Shared: feels like it was written in another language (to be fair it probably was originally)
IFunctions ~- SorghumArbitrator (and possibly other parts of sorghum too, will investigate)~

I may update this as I find more problems. Mainly posting this so that I don't forget about it.

~- Summary shouldn't check the soil. Soil should check the soil (see Summary.cs:86)~ ~- BiomassTypeArbitrator.OnSimulationCommencing~

Every implementation of IRetranslocationMethod has an identical implementation of void Document(...)
Debug statement in Links.cs
Unused variable warning in clem

jbrider commented 4 years ago

SorghumArbitrator should now be redundant and can be deleted.

hol430 commented 4 years ago

@hol353 I notice that we have a UserInterface directory in the repo - as far as I can tell, this appears to be the legacy winforms UI which was deleted several years ago but was accidentally reintroduced in a dodgy merge (9e6a1c46e422bab9d20734d79ba83a4bc473f18c). Is this correct? If so I will go and delete it again.

hol353 commented 4 years ago

Yep. That's a legacy directory. You can delete it.

hol430 commented 4 years ago

As part of our work on .net core builds, we've had to explicitly remove several files from compilation in Models.csproj. This is because none of these files are included in the Models project in the master branch, and they do not compile. You might be surprised to hear that DataStore.cs doesn't compile, but we seem to have 2 copies of DataStore.cs, one in Models/DataStore.cs, and one in Models/Storage/DataStore.cs. @hol353 can all of these files all be removed?

  <ItemGroup>
    <Compile Remove="DataStore.cs" />
    <Compile Remove="Functions\BoundFunction.cs" />
    <Compile Remove="GRange.cs" />
    <Compile Remove="Lifecycle\LifeStage.cs" />
    <Compile Remove="Lifecycle\LifeStageGrowth.cs" />
    <Compile Remove="Lifecycle\LifeStageImmigration.cs" />
    <Compile Remove="Lifecycle\LifeStageMortality.cs" />
    <Compile Remove="Lifecycle\LifeStageReproduction.cs" />
    <Compile Remove="Lifecycle\LifeStageTransfer.cs" />
    <Compile Remove="Plant\BiomassSummariser.cs" />
    <Compile Remove="Plant\Phenology\PhotoperiodEffect.cs" />
    <Compile Remove="Plant\Phenology\VernalisationEffect.cs" />
    <Compile Remove="Report\RowEnumerator.cs" />
  </ItemGroup>

hol353 commented 4 years ago

If they are not included in the Models project then yes they can be removed.

hol430 commented 4 years ago

Another thing which could be tidied up is the PropertyTreeView and other classes in ApsimNG which are essentially copies of other classes. Having to go and fix the same bug multiple times is a pain, and I'm sure I've missed a few since these classes were added. It would be good if we could find some way to extract the shared code, perhaps into a shared class, or a base class.

APSIMInitiative / ApsimX

Some tidying up is needed #5334