8
\$\begingroup\$

Each product in my e-commerce website has Arabic and English values for the title, description and excerpt. I have this method EditProduct to update those values based on current culture (Arabic or English)

public void EditProduct(string element, string text1, string text2, bool edit1, bool edit2, string culture)
{
 var product = DoSomeMagicToGetAProduct();
 if (element == "title")
 {
 if (culture == "en")
 {
 if (edit1)
 {
 product.Title = text1;
 }
 if (edit2)
 {
 product.TitleAr = text2;
 }
 }
 if (culture == "ar")
 {
 if (edit1)
 {
 product.TitleAr = text1;
 }
 if (edit2)
 {
 product.Title = text2;
 }
 }
 }
 if (element == "description")
 {
 // similar codes here
 }
 if (element == "excerpt")
 {
 // similar codes here
 }
 product.Save();
}

The method works well, but I think I can improve the way I write it to be more elegant. Any suggestions?

Jamal
35.2k13 gold badges134 silver badges238 bronze badges
asked Aug 9, 2011 at 21:30
\$\endgroup\$
2
  • \$\begingroup\$ What are Title and TitleAr for? What do edit1 and edit2 mean? \$\endgroup\$ Commented Aug 9, 2011 at 22:00
  • \$\begingroup\$ @Winston TitleAr is the arabic value of Title (english value). edit1 == true means user is editing text1. \$\endgroup\$ Commented Aug 10, 2011 at 7:59

3 Answers 3

11
\$\begingroup\$

I think the code is a little dodgy, and the design is dodgy. I'd store the translation in a dictionary of some sort.

But addressing your immediate question, to simplify the code you have, you could do....

public void EditProduct(string element, string text1, string text2, bool edit1, bool edit2, string culture)
 {
 var product = DoSomeMagicToGetAProduct();
 var arText = culture == "ar" ? text1 : text2;
 var enText = culture == "en" ? text1 : text2;
 var updateEn = culture == "en" ? edit1 : edit2;
 var updateAr = culture == "ar" ? edit1 : edit2;
 if (element == "title")
 {
 if (updateEn) product.Title = enText;
 if (updateAr) product.TitleAr = arText; 
 }
 if (element == "description")
 {
 if (updateEn) product.Description = enText;
 if (updateAr) product.DescriptionAr = arText; 
 }
 if (element == "excerpt")
 {
 if (updateEn) product.Excerpt = enText;
 if (updateAr) product.ExcerptAr = arText; 
 }
 product.Save();
 }
answered Aug 10, 2011 at 1:22
\$\endgroup\$
3
  • \$\begingroup\$ Thank you very much, Nicholas. this looks better than mine. I also appreciate your suggestion on my design, but I'm afraid storing those values in some kind of dictionary is too much, since the website only support arabic & english. \$\endgroup\$ Commented Aug 10, 2011 at 7:55
  • \$\begingroup\$ This certainly does tidy up the code, however removing parentheses could possibly contravene any coding standards in place. @Anwar Chandra I would suggest reading page 23 & 24 of C# Code Style Guide sourceformat.com/pdf/cs-coding-standard-bellware.pdf for inspiration. \$\endgroup\$ Commented Aug 10, 2011 at 9:50
  • \$\begingroup\$ instead of the ifs, use switch case statements. \$\endgroup\$ Commented Aug 10, 2011 at 12:59
1
\$\begingroup\$

You should try using a switch statement for the languages. Also move the languages to be the first statements to check instead of the element. This combined should reduce the size of your code.

http://msdn.microsoft.com/en-us/library/06tc147t%28v=vs.80%29.aspx

Then for the edit booleans you can do this instead.

 if (edit1)
 {
 product.Title = text1;
 }
 else 
 {
 product.TitleAr = text2;
 }
answered Aug 9, 2011 at 22:02
\$\endgroup\$
1
\$\begingroup\$

Why should you need if-else for culture change? .Net resources is the way to show localized web pages as per culture on user machine. You should have just one product.title field and 2 resource files, one neutral culture which contains english words and another 'ar' culture which holds the arabic translation.

Refer to http://msdn.microsoft.com/en-us/library/ms227427.aspx for an overview of asp.net resources.

Hope this helps. If I am missing something here please post.

answered Aug 10, 2011 at 5:10
\$\endgroup\$
1
  • \$\begingroup\$ Yes, localization in the website already handled by .Net resources. product isn't resource, and my users need to input its values manually on a daily basis. \$\endgroup\$ Commented Aug 10, 2011 at 7:56

Your Answer

Draft saved
Draft discarded

Sign up or log in

Sign up using Google
Sign up using Email and Password

Post as a guest

Required, but never shown

Post as a guest

Required, but never shown

By clicking "Post Your Answer", you agree to our terms of service and acknowledge you have read our privacy policy.

Start asking to get answers

Find the answer to your question by asking.

Ask question

Explore related questions

See similar questions with these tags.