external-repos/forge
1
0
Fork 0
mirror of https://github.com/Card-Forge/forge.git synced 2025-11-18 03:38:01 +00:00
Code Issues Packages Projects Releases Wiki Activity

Comment (and test!) to demonstrate that current implementation of getEarliestEditionWithAllCards not only has a misleading name, but also doesn't work the way it should!

Browse Source
This commit is contained in:
leriomaggio
2021-06-09 17:48:07 +01:00
parent 58b2a96168
commit 448263d9de
2 changed files with 74 additions and 40 deletions
Download Patch File Download Diff File Expand all files Collapse all files

78
forge-core/src/main/java/forge/card/CardEdition.java
View File

@@ -17,49 +17,27 @@
*/ */
package forge.card; package forge.card;
import java.io.File;
import java.io.FilenameFilter;
import java.text.SimpleDateFormat;
import java.util.ArrayList;
import java.util.Arrays;
import java.util.Calendar;
import java.util.Collections;
import java.util.Comparator;
import java.util.Date;
import java.util.HashMap;
import java.util.HashSet;
import java.util.List;
import java.util.Locale;
import java.util.Map;
import java.util.Map.Entry;
import java.util.Set;
import java.util.TreeMap;
import java.util.regex.Matcher;
import java.util.regex.Pattern;
import org.apache.commons.lang3.StringUtils;
import com.google.common.base.Function; import com.google.common.base.Function;
import com.google.common.base.Predicate; import com.google.common.base.Predicate;
import com.google.common.collect.ArrayListMultimap; import com.google.common.collect.*;
import com.google.common.collect.ImmutableList;
import com.google.common.collect.Iterables;
import com.google.common.collect.ListMultimap;
import com.google.common.collect.Lists;
import forge.StaticData; import forge.StaticData;
import forge.card.CardDb.SetPreference; import forge.card.CardDb.CardArtPreference;
import forge.deck.CardPool; import forge.deck.CardPool;
import forge.item.PaperCard; import forge.item.PaperCard;
import forge.item.SealedProduct; import forge.item.SealedProduct;
import forge.util.Aggregates; import forge.util.*;
import forge.util.FileSection;
import forge.util.FileUtil;
import forge.util.IItemReader;
import forge.util.MyRandom;
import forge.util.storage.StorageBase; import forge.util.storage.StorageBase;
import forge.util.storage.StorageReaderBase; import forge.util.storage.StorageReaderBase;
import forge.util.storage.StorageReaderFolder; import forge.util.storage.StorageReaderFolder;
import org.apache.commons.lang3.StringUtils;
import java.io.File;
import java.io.FilenameFilter;
import java.text.SimpleDateFormat;
import java.util.*;
import java.util.Map.Entry;
import java.util.regex.Matcher;
import java.util.regex.Pattern;
/** /**
@@ -460,7 +438,7 @@ public final class CardEdition implements Comparable<CardEdition> { // immutable
/* /*
The following pattern will match the WAR Japanese art entries, The following pattern will match the WAR Japanese art entries,
it should also match the Un-set and older alternate art cards it should also match the Un-set and older alternate art cards
like Merseine from FEM (should the editions files ever be updated) like Merseine from FEM.
*/ */
//"(^(?<cnum>[0-9]+.?) )?((?<rarity>[SCURML]) )?(?<name>.*)$" //"(^(?<cnum>[0-9]+.?) )?((?<rarity>[SCURML]) )?(?<name>.*)$"
/* Ideally we'd use the named group above, but Android 6 and /* Ideally we'd use the named group above, but Android 6 and
@@ -713,16 +691,36 @@ public final class CardEdition implements Comparable<CardEdition> { // immutable
}; };
} }
/* @leriomaggio [Note to self]
I am sure this method does not work the way it is expected - do some tests
The name is also misleading!
What the method **really** does is to return the **latest** (as in the most recent)
Card Edition among the different (see Set<String>) sets found
that includes cards in the pool, picked by an "Old Art First" policy.
There is definitely no-guarantee that **all** cards in the pool will be
included in the set returned!
CounterExample: All cards from LEA, one card from USG.
USG will be returned!
See CardEditionCollectionTestCase.getEarliestEditionWithAllCardsNotWorkingAsExpected
test case that tackles exactly this example!
What I would do instead is to return the set containing the majority of the cards in
the pool (still picked with the same "Old Art First" policy!
A good renaming would be "getEarlierEditionWithMostOfTheCards"
*/
public CardEdition getEarliestEditionWithAllCards(CardPool cards) { public CardEdition getEarliestEditionWithAllCards(CardPool cards) {
Set<String> minEditions = new HashSet<>(); Set<String> minEditions = new HashSet<>();
SetPreference strictness = SetPreference.EarliestCoreExp; CardArtPreference strictness = CardArtPreference.OldPrintNoPromoNoOnline;
for (Entry<PaperCard, Integer> k : cards) { for (Entry<PaperCard, Integer> k : cards) {
PaperCard cp = StaticData.instance().getCommonCards().getCardFromEdition(k.getKey().getName(), strictness); PaperCard cp = StaticData.instance().getCommonCards().getCardFromEditions(k.getKey().getName(), strictness);
if( cp == null && strictness == SetPreference.EarliestCoreExp) { if( cp == null && strictness == CardArtPreference.OldPrintNoPromoNoOnline) {
strictness = SetPreference.Earliest; // card is not found in core and expansions only (probably something CMD or C13) strictness = CardArtPreference.OldPrint; // card is not found in core and expansions only (probably something CMD or C13)
cp = StaticData.instance().getCommonCards().getCardFromEdition(k.getKey().getName(), strictness); cp = StaticData.instance().getCommonCards().getCardFromEditions(k.getKey().getName(), strictness);
} }
if ( cp == null ) if ( cp == null )
cp = k.getKey(); // it's unlikely, this code will ever run cp = k.getKey(); // it's unlikely, this code will ever run

36
forge-gui-desktop/src/test/java/forge/card/CardEditionCollectionTestCase.java Normal file
View File

@@ -0,0 +1,36 @@
package forge.card;
import forge.deck.CardPool;
import forge.item.PaperCard;
import forge.model.FModel;
import org.testng.annotations.Test;
import java.util.ArrayList;
import java.util.List;
import static org.testng.Assert.assertEquals;
public class CardEditionCollectionTestCase extends ForgeCardMockTestCase {
@Test
public void getEarliestEditionWithAllCardsNotWorkingAsExpected(){
CardEdition.Collection editions = FModel.getMagicDb().getEditions();
CardDb cardDb = FModel.getMagicDb().getCommonCards();
String[] cardNames = {"Shivan Dragon", "Animate Wall", "Balance", "Blessing", "Force of Will"};
String[] expectedSets = {"LEA", "LEA", "LEA", "LEA", "ALL"};
List<PaperCard> cards = new ArrayList<>();
for (int i=0; i < 5; i++){
String cardName = cardNames[i];
String expectedSet = expectedSets[i];
PaperCard card = cardDb.getCardFromEditions(cardName, CardDb.CardArtPreference.OldPrint);
assertEquals(card.getEdition(), expectedSet);
cards.add(card);
}
CardPool pool = new CardPool();
pool.add(cards);
CardEdition ed = editions.getEarliestEditionWithAllCards(pool);
assertEquals(ed.getCode(), "LEA");
}
}