0
\$\begingroup\$

So I started learning OOs and code refactoring and decided to do some project. It just some simple quiz game.

Here are some mechanics

  • There are 3 sets

  • Each set have 8 Brands (types)

  • Each brand have 10 questions

So what I did was this

Set Class

public class Set{
    public string setName;
    public Brand[] brands;
}

Brand Class

public class Brand{
    public string brandName;
    public Question[] questions;
}

Question Class

public class Question {
    public string question;
    public string[] choices = new string[3];  //choices for each question
    public string correctAnswer;  //will hold the correct answer for a question

    public string questionID(){
       // making a id for question
        string qID = "";

        qID += question + "|";

        foreach(string choice in choices){
            qID += choice + "|";
        }

        qID += correctAnswer + "|";

        qID += fontSize;

        return qID;
    }

}

So what I need is to create some kind of id for each question, what I did is this

public class QuestionManager : MonoBehaviour {
public void saveToDB(){
    int setID = 0;
      foreach(Set set in sets){

          int brandID = 0;
          foreach(Brand brand in set.brands){

              int questionID = 0;
             foreach(Question question in brand.questions){


                string questionKey = "s" + setID + "|" + "b" + brandID + "|" + "q" + questionID;
                string questionAnswer =  "value:"+ set.setName + "|" + brand.brandName + "|" + question.questionID() ;

                PlayerPrefs.SetString(
                key: questionKey,
                value: questionAnswer
                );  // save to the database

                questionID++; 
          }
            brandID++;
          }
          setID++;
      }

}
}

I'm really curious if there are some good refactoring for this kind of loop. Also I should add that I'll be using this ids for getting the questions later.

\$\endgroup\$
  • 1
    \$\begingroup\$ Before anyone can write any review we must know what your code is doing. Currently we only know it's quiz. Could you explain how it works by editing your quesiton and adding this information to it? \$\endgroup\$ – t3chb0t Oct 29 '18 at 8:49
  • \$\begingroup\$ I tried adding comments and more description. This is simply a code for making questionId. And use that id to navigate the questions later. \$\endgroup\$ – naisu Oct 29 '18 at 8:56
  • 1
    \$\begingroup\$ Welcome to Code Review! The current question title, which states your concerns about the code, is too general to be useful here. Please edit to the site standard, which is for the title to simply state the task accomplished by the code. Please see How to get the best value out of Code Review: Asking Questions for guidance on writing good question titles. \$\endgroup\$ – Toby Speight Oct 29 '18 at 9:04
  • \$\begingroup\$ saveToDB is supposed to save something to a database, right? Why isn't it doing anything then? You are creating a string that is never used anywhere... \$\endgroup\$ – t3chb0t Oct 29 '18 at 9:26
  • \$\begingroup\$ There I added the codes. I thought that will be enough since I'm just here asking for the nested loop refactoring. \$\endgroup\$ – naisu Oct 29 '18 at 9:30
2
\$\begingroup\$

You ask for a refactoring of the current method. It could be:

public void saveToDB()
{
  int setID = 0;
  foreach (Set set in sets)
  {
    int brandID = 0;
    foreach (Brand brand in set.brands)
    {
      int questionID = 0;
      foreach (Question question in brand.questions)
      {
        PlayerPrefs.SetString(
          key: $"s{setID}|b{brandID}|q{questionID}",
          value: $"value:{set.setName}|{brand.brandName}|{question.questionID()}"); // save to the database

        questionID++;
      }
      brandID++;
    }
    setID++;
  }
}

Here is used string interpolation $"s{setID}...." instead of string concatenation which is considered to be bad practice because it results in too many string instantiations and is hard to read.


It could be done by using LINQ, but that is maybe more confusing and will actually be slower:

public void saveToDB()
{
  var questionInfos = sets.SelectMany((set, setId) => set.brands.SelectMany((brand, brandId) => brand.questions.Select((question, questionId) => new { key = $"s{setId}|b{brandId}|q{questionId}", value = $"value:{set.setName}|{brand.brandName}|{question.questionID()}" })));

  foreach (var questionInfo in questionInfos)
  {
    PlayerPrefs.SetString(questionInfo.key, questionInfo.value);
  }
}

Just for the sport, it can be done in inline query syntax (with a little help from method syntax) as t3echb0t suggested:

public void saveToDB()
{
  var questionInfos = from setAndId in sets.Select((set, id) => (set, id))
                      from brandAndId in setAndId.set.brands.Select((brand, id) => (brand, id))
                      from questionAndId in brandAndId.brand.questions.Select((question, id) => (question, id))
                      select new
                      {
                        key = $"s{setAndId.id}|b{brandAndId.id}|q{questionAndId.id}",
                        value = $"value:{setAndId.set.setName}|{brandAndId.brand.brandName}|{questionAndId.question.questionID()}"
                      };

  foreach (var questionInfo in questionInfos)
  {
    PlayerPrefs.SetString(questionInfo.key, questionInfo.value);
  }
}

That said, I would consider an mandatory Id to be a property on each Set, Brand and Question instance:

public abstract class ObjectBase
{
  public ObjectBase(string name, int id)
  {
    Name = name;
    Id = id;
  }

  public string Name { get; }
  public int Id { get; }
}

public class Set : ObjectBase
{
  public Set(string name, int id) : base(name, id)
  {

  }

  public Brand[] Brands { get; set; }
}

public class Brand : ObjectBase
{
  public Brand(string name, int id) : base(name, id)
  {

  }

  public Question[] Questions { get; set; }
}

public class Question : ObjectBase
{
  public int FontSize;
  public string[] Choices = new string[3];  //choices for each question
  public readonly int CorrectAnswerId;  //will hold the correct answer for a question

  public Question(string question, int correctAnswerId, string[] choices, int id) : base(question, id)
  {
    CorrectAnswerId = correctAnswerId;
    Choices = choices;
  }

  public string TheQuestion => Name;

  public string QuestionID()
  {
    // making a id for question
    return $"{TheQuestion}|{string.Join("|", Choices)}|{CorrectAnswerId}|{FontSize}";
  }

}


public class QuestionManager //: MonoBehaviour
{
  Set[] sets = new Set[0];

  public void saveToDB()
  {
    foreach (Set set in sets)
    {
      foreach (Brand brand in set.Brands)
      {
        foreach (Question question in brand.Questions)
        {
          PlayerPrefs.SetString(
            key: $"s{set.Id}|b{brand.Id}|q{question.Id}",
            value: $"value:{set.Name}|{brand.Name}|{question.QuestionID()}"); // save to the database
        }
      }
    }
  }
}

Notice here that there is no point in calling the name of a Set "setName - Name is sufficient. Further notice that it is common practice in C# to use PascalCase for public members.

The benefit of having the entities be born with an id, is that you can use that id to identify each instance uniquely throughout the session/lifetime of each object. And as shown the key/value-pair for PlayerPrefs.SetString is easier created.

\$\endgroup\$
  • \$\begingroup\$ If you write linq in a single line like you did then it's a sure thing it's confusing ;-P in cases with multiple SelectMany the query syntax is much more readable - of course only when you use line breaks ;-] \$\endgroup\$ – t3chb0t Oct 29 '18 at 11:52
  • \$\begingroup\$ @t3chb0t: I'm not that used to write inline queries, and I'm not sure how to include an index. If you know how feel free to add to my answer. \$\endgroup\$ – Henrik Hansen Oct 29 '18 at 12:02
  • \$\begingroup\$ Nah, maybe another time with a better quesiton. I don't feel like figuring out what sets is - I told OP to provide everything but they refused so I don't care - almost forgot to give a +1 for actually doing this ;-) \$\endgroup\$ – t3chb0t Oct 29 '18 at 12:08

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.