2
\$\begingroup\$

I'm writing a text adventure editor that identifies various StoryEntity instances by name, among other means, but the name is the primary way for the user or player to refer to them. The name doesn't have to be unique. For example, there may by many instances of "North" which each refer to a different StoryEntity depending on where the player is, or the context of which container you're referring to. Sometimes I know the name will be ambiguous, but I want to give preference to names of entities that are near the player. Other times, like when the user types Edit North, they may be dealing with a design-mode copy of the data where the player is always in its designed starting location, but I have another way of identifying the "current location". Still other times, I don't want to waste time sorting the results at all because I will handle all of them equally. So I updated the function that retrieves a StoryEntity by name to include a parameter indicating whether and how to sort the results by relevance:

public IEnumerable<StoryEntity> EntitiesNamed(string name,
   RelevantContainer relevantContainer)
{
   if (relevantContainer == RelevantContainer.PlayerAndContainer)
      return byName[name.ToLower()].OrderBy((e) => (e.Container == PLAYER_ENTITYID)
         ? 1 : ((e.Container == Player.Container) ? 2 : 3));
   else if (relevantContainer > 0)
      return byName[name.ToLower()].OrderBy((e) => ((e.Container == relevantContainer)
         ? 2 : 3));
   else
      return byName[name.ToLower()];
}

Now my question is how can I best define RelevantContainer so that it allows pre-defined values as well as arbitrary int (container ID) values? I can imagine implementing it as an enum or a struct:

Struct

public struct RelevantContainer
{
   private int value;
   public static implicit operator RelevantContainer(int containerId)
   {
      return new RelevantContainer() { value = containerId };
   }
   public static implicit operator Int32(RelevantContainer relevantContainer)
   {
      return relevantContainer.value;
   }
   public readonly static RelevantContainer None = 0;
   public readonly static RelevantContainer PlayerAndContainer = -1;
}

Enum

public enum RelevantContainer
{
   None = 0,
   PlayerAndContainer = -1
}

Of course if I implement it as an enum, then I will have to do more casting to persuade the compiler to allow integer values and enum values to be interchanged, but the code for RelevantContainer itself is much simpler. Is there a clear choice for which approach to use here? If it matters, this is a Xamarin Android project.

\$\endgroup\$
  • \$\begingroup\$ Why not split this method up? Have one overload accept a sorting enum and another one accept a container ID. \$\endgroup\$ – Pieter Witvoet Jan 1 '18 at 13:53
  • \$\begingroup\$ @PieterWitvoet That probably would have been a good idea had I not needed to rewrite the EntitiesNamed function to a more readable implementation, as suggested by apieceoffruit. \$\endgroup\$ – BlueMonkMN Jan 1 '18 at 14:04
  • \$\begingroup\$ Shall I conclude that any desire to mix enum values and arbitrary integers should be a signal to find a different implementation, of which there appear to be many alternatives? \$\endgroup\$ – BlueMonkMN Jan 1 '18 at 14:06
  • \$\begingroup\$ The problem is not that they're of a different type, but that they serve different purposes. It's probably not a good idea to mix first and last names either, even though both can be stored in a string. \$\endgroup\$ – Pieter Witvoet Jan 1 '18 at 14:21
2
\$\begingroup\$

Unless I am mistaken you are dealing with a int32, string key value pair, why not use a simple struct consisting of an int and a string?

although as this is CodeReview I have to say, your use of ternary operators nested in linq statements with more terniaries is damn near illegible.


Don't forget, code you wrote 6 months ago is indistinguishable from strangers code.

If you saw this code and had never seen it before you would think it is clear and makes sense?

to be polite that EntitiesNamed method is mostly gibberish.

You are trying to define relevance in your story entities, I get it, but a hard coded nest of grouping ids is scary to read and very easy to mess up.

you are basically talking about a rule engine, Given a name and a gamestate, find the right StoryEntities.

A level down from that is, if the current gamestate and name match a certain condition, filter the returned Entities.

then you are, based on conditions grouping those entities by relevance.

you method is doing a search, a filtering and a conditional grouping, these are different tasks and need to at least be method separate, certainly not be all on one or two lines.

what if you decide to swap return order?


Remember, great code looks like well written prose, would something like

var storyentities = GetEntitiesByName(entityName);
var orderedentities = OrderByContainer(storyentities,container);

not make more sense?

could the OrderByContainer method not even be a method on a ContainerOrderer interface of some kind defining the rules for deciding relevancy? would that not separate your concerns, make testing easier and over all make it clearer what each piece of your code is doing?

\$\endgroup\$
  • \$\begingroup\$ My goal with incorporating the sorting into GetEntitiesByName was to enforce compulsory almost-implicit sorting because when calling the function, I'm not thinking about how to get the most relevant result. I don't want to forget to order the results in some cases and end up with some cases where I'll get a less relevant result because I forgot that I always need to consider sorting them. I see your point about illegible LINQ expressions. But what's LINQ for if not to make code more compact. I may confuse compactness with elegance (a common pitfall?) What would you do about this example? \$\endgroup\$ – BlueMonkMN Dec 31 '17 at 18:27
  • \$\begingroup\$ That reasoning though is called 'siloing' because you alone know that, nothing about the public name of the method implies anything is sorted, or that the sorting is important. If you want the sorting to be so explicit, you need a GetEntitiesSortedByRelevance(string name) method, clunky but clear, design should not require hidden knowledge. as for linq usage, link can take method calls, inline lambdas look cool but basically just cause mess, the chain _entities.OrderBy(Relevance) does the same thing but actually reads clearly. just define a void Relevance(StoryEntity e) \$\endgroup\$ – apieceoffruit Dec 31 '17 at 19:44
  • \$\begingroup\$ also as an aside, the point of linq is actually fluent syntax. in short it is designed to make code look MORE like english, not less. _people.Where(OldEnoughToDrink).GroupBy(FavouriteBars).OrderBy(x=>x.LastName).Select(AsPartyInvite) is a succinct way to perform a number of tasks on a collection without intermediate collections, lazy evaluated, returned as a new entity and easy to read. Just because you can inline doesn't mean you should. linq should make your code easier to read, not harder \$\endgroup\$ – apieceoffruit Dec 31 '17 at 19:48
  • \$\begingroup\$ I added another answer based on your suggestions (I tried to add it to your answer, but that edit got rejected). I am wondering if it addresses the issues you pointed out as you intended. \$\endgroup\$ – BlueMonkMN Dec 31 '17 at 23:54
0
\$\begingroup\$

I have devised the following solution based on apieceoffruit's answer, and am interested to know if this is a step in the right direction and if there are further refinements to make. (I tried to apply this as an edit to that answer, but that got rejected -- too much new content I guess.)

The new implementation of EntitiesNamed looks like this:

public IEnumerable<StoryEntity> EntitiesNamed(string name, RelevanceOrder relevance)
{
   if (relevance != null)
      return byName[name].OrderBy(relevance.Relevance);
   else
      return byName[name];
}

The new implementation for the type of the second parameter looks like this:

public class RelevanceOrder
{
   StoryEntity[] containerPrecedence;

   public RelevanceOrder(StoryEntity container)
   {
      containerPrecedence = new StoryEntity[] { container };
   }

   public RelevanceOrder(StoryEntity[] containerPrecedence)
   {
      this.containerPrecedence = containerPrecedence;
   }

   public static RelevanceOrder ByPlayerThenLocation(StoryData story)
   {
      return new RelevanceOrder(new StoryEntity[] { story.Player, story.PlayerWrapper.GetContainer() });
   }

   public int Relevance(StoryEntity entity)
   {
      int result = Array.IndexOf(containerPrecedence, entity);
      return result < 0 ? int.MaxValue : result;
   }
}

It then gets called in statements like this:

return StoryData.EntitiesNamed(tokens[tokenIndex].GetTokenText(),
   RelevanceOrder.ByPlayerThenLocation(StoryData)).FirstOrDefault();
...
if (context.StoryData.EntitiesNamed(command.Substring(start)
   .Trim(), null).Intersect(scope(context)).Any())
...
EntityEditor ee = new EntityEditor(resetStory, users, editEntity, context.GetString(Resource.String.title_edit_entity),
   EntityEditor.EditFlags.All, new RelevanceOrder(storyData.PlayerWrapper.GetContainer()));

I didn't understand how you could have a void function be a parameter to OrderBy, so this was the next best thing I could work out. Also, I don't know if it seems redundant to pass a StoryData-based object to a StoryData function as a parameter rather than assume the same context, but there are cases (the last example above) where the player and container information need to come from a different instance of StoryData due to the different player location, so I think it makes sense.

\$\endgroup\$
  • 1
    \$\begingroup\$ nice, far easier to tell intent. Now things are easier to read we can address a few small things,e.g properties are nouns: Employee[] Employees , but methods are verbs. GetEmployeesByFirstName(string firstName) , I'd recommend revisiting EntitesNamed's name. you can also write EntitiesNamed(string name, RelevanceOrder relevance = null) to avoid needing to pass a null. My design suggestion would be a Fascade/UnitOfWork. The wiring is complex enough a friendly named wrapper over parts would aid understanding. e.g relationship between tokens and entities, maybe factory the Editor. \$\endgroup\$ – apieceoffruit Jan 1 '18 at 20:52
  • \$\begingroup\$ The downvote is for accepting your own answer and not the one that helped you. \$\endgroup\$ – t3chb0t Jan 6 '18 at 19:26
  • \$\begingroup\$ @t3chb0t I would have accepted the answer that helped me, but when I tried to complete it with the information in my own answer so that I could do so, someone rolled back the changes. So I did the next best thing (per instructions of the reviewer who rolled back my changes) - up-voted the answer that helped me, and separately posted my actual solution that I used. Are you saying that I should separately post the actual solution I used, but accept the answer that does not contain my final solution? \$\endgroup\$ – BlueMonkMN Jan 8 '18 at 14:04

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.