3
\$\begingroup\$

I have a function which sets the text to a label displaying current active "products" and current active pages. The label displays "X products, Y pages/page" depending on the amount of products active in the template. Products is the same as rows really, the client wants to call it products.

The template consists of a number of tags, activeTags, which holds the tags that are not deleted. One row consists of a number of tags, perhaps 2 or 3, in the property _selectedTemplateConfiguration.TemplateConfigurationTagsPerRow. Every template page can hold a number of products/rows, _selectedTemplateConfiguration.TemplateConfigurationRowsPerPage.

Let's say I have 41 rows and the template takes 20 rows on each page. This means I have 3 pages active. When I made the function I could not get it to work without the use of the divisionRestvariable, ie. when I had 40 rows it still said it was 3 active pages, due to Math.Ceiling.

The _currentTagCollectionconsists really of a number of Telerik RadDocks with a text input, the idea with the whole template is that the client can enter textvalues into a template online, which is the printed.

I probably missed something, but this is a function that I feel could be made different. Feel free to ask if I need to clarify anything. I'm using .NET 4.0.

enter image description here

private string displayTemplateNumbers()
{
  int totalPagesActive;
  var activeTags = _currentTagCollection.Count - totalAmountOfDeletedTags();

  var divisionRest = (Convert.ToDouble(activeTags / _selectedTemplateConfiguration.TemplateConfigurationTagsPerRow) % _selectedTemplateConfiguration.TemplateConfigurationRowsPerPage);

  if (divisionRest == 0) //Math.Ceiling thinks it's 3 pages when it's only 2, have to check divisionRest like this
  {
       totalPagesActive = Convert.ToInt32((activeTags / _selectedTemplateConfiguration.TemplateConfigurationRowsPerPage) / _selectedTemplateConfiguration.TemplateConfigurationTagsPerRow);
  }
  else
  {
       totalPagesActive = Convert.ToInt32(Math.Ceiling((Convert.ToDouble(activeTags) / Convert.ToDouble(_selectedTemplateConfiguration.TemplateConfigurationRowsPerPage)) / Convert.ToDouble(_selectedTemplateConfiguration.TemplateConfigurationTagsPerRow)).ToString());
  }
  var pages =  totalPagesActive > 1 ? string.Format("{0} pages", totalPagesActive) : string.Format("{0} page", 1);
  return string.Format("{0} products, {1}", activeTags / _selectedTemplateConfiguration.TemplateConfigurationTagsPerRow, pages);
}

private int totalAmountOfDeletedTags()
{
 int counter = 0;
 foreach (var tag in _currentTagCollection)
 {
   if (tag.Deleted)
   {
      counter++;
   }
 }
 return counter;
}
\$\endgroup\$
  • 2
    \$\begingroup\$ What version of .net are you using? What is T in List<T> _currentTagCollection? Can you please post private int totalAmountOfDeletedTags() \$\endgroup\$ – Robert Snyder Feb 3 '16 at 13:01
  • 2
    \$\begingroup\$ @RobertSnyder Edited my question with version and the missing function + screenshot, trying to make it a little clearer. \$\endgroup\$ – Magnetize Feb 3 '16 at 13:16
3
\$\begingroup\$

Issue with ceiling is just misprinting.
You should call Ceiling on whole expression, not just on part.
So instead

totalPagesActive = Convert.ToInt32(Math.Ceiling((Convert.ToDouble(activeTags) / Convert.ToDouble(_selectedTemplateConfiguration.TemplateConfigurationRowsPerPage)) / Convert.ToDouble(_selectedTemplateConfiguration.TemplateConfigurationTagsPerRow)).ToString());

should be

totalPagesActive = Convert.ToInt32(Math.Ceiling((Convert.ToDouble(activeTags) / Convert.ToDouble(_selectedTemplateConfiguration.TemplateConfigurationRowsPerPage) / Convert.ToDouble(_selectedTemplateConfiguration.TemplateConfigurationTagsPerRow))).ToString());

and you can remove if blocks after this

Here is test with ceiling with your values:
https://dotnetfiddle.net/19FESY

\$\endgroup\$
  • \$\begingroup\$ Oh man, stupid ). Seems to work like a charm, thanks a lot. :) \$\endgroup\$ – Magnetize Feb 3 '16 at 13:37
  • \$\begingroup\$ Hm, I noticed after some tests that if I test this with your fiddle, with three variables, it does not work. Do you have any idea why that is? dotnetfiddle.net/KH091J \$\endgroup\$ – Magnetize Feb 3 '16 at 14:28
  • \$\begingroup\$ @Magnetize, In your fiddle you again put brackets as in original sample. Try to simplify your expression in code. totalPagesActive = (int)Math.Ceiling(((double)40) / 20 / 2); dotnetfiddle.net/pZi1Ih \$\endgroup\$ – hardsky Feb 3 '16 at 14:42
  • \$\begingroup\$ Hm I still dont get it though. var test = (int)Math.Ceiling(((double)40) / 20 / 2);should show 2 pages. var test = (int)Math.Ceiling(((double)41) / 20 / 2); should show 3 pages. Am I just doing it totally wrong? dotnetfiddle.net/DrJaej \$\endgroup\$ – Magnetize Feb 3 '16 at 14:51
  • \$\begingroup\$ @Magnetize, if there are should be 2 and 3 pages, then error in logic somewhere else. Because 40 / 20 / 2 = 1, same as 2x2=4 \$\endgroup\$ – hardsky Feb 3 '16 at 15:12
3
\$\begingroup\$

Your method names do not follow capitalization standards, they should be PascalCase.

However, what's worse is that their names do not correspond to that they do:

  • displayTemplateNumbers() returns a formatted string (and thus doesn't "display" anything),
  • totalAmountOfDeletedTags() feels like it should be a property instead of a method.

And totalAmountOfDeletedTags() isn't even necessary, since it can be replaced by simple LINQ: _currentTagCollection.Count(x => x.Deleted);. And since you only need the active tags, you can simply do var activeTags = _currentTagCollection.Count(x => !x.Deleted); (h/t to @RobertSnyder).


At the end of displayTemplateNumbers() you twice call string.Format(). Instead you should do this once:

return string.Format("{0} products, {1} page{2}", 
    activeTags / _selectedTemplateConfiguration.TemplateConfigurationTagsPerRow, 
    totalPagesActive,
    (totalPagesActive > 1 ? "s" : string.Empty));
\$\endgroup\$
  • 1
    \$\begingroup\$ OP wants active tags, so Count(x=>!x.Deleted) \$\endgroup\$ – Robert Snyder Feb 3 '16 at 13:23
  • 1
    \$\begingroup\$ @RobertSnyder The method is called totalAmountOfDeletedTags() and his own logic clearly counts tags where tag.Deleted is true. Ah wait, I see what you mean, you're right, that line can be shorter. \$\endgroup\$ – BCdotWEB Feb 3 '16 at 13:24
  • \$\begingroup\$ @BCdotWEB thanks, this is part of a inhouse CMS that I happened to inherit. They use camelCase for private methods and PascalCase for public methods, that's why I kept it that way. I see your point with the method names, and I thank you for the suggestions. :) \$\endgroup\$ – Magnetize Feb 3 '16 at 13:30

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.