-2
\$\begingroup\$

I am doing a coding exercise on leetcode, but wondering, is this a good coding? Please help check https://leetcode.com/problems/text-justification/discuss/2017233

Here is the code copy:

class Solution{
private:
    int wordsCnt;
    int charCnt;
    int emptyCnt;
    vector<string> res;
    void pairing(vector<string>& words, int maxWidth, int startIndex);
    void centerJustify(vector<string>& words, int maxWidth, int startIndex);
    void leftJustify(vector<string>& words, int maxWidth, int startIndex);
    
public:
    vector<string> fullJustify(vector<string>& words, int maxWidth);
};


// Given the beginning index, 
// 1. Get how many words is needed to build a line -> wordsCnt
// 2. Get how many empty space should be added -> emptyCnt
// 3. Get how many characters are in the line -> charCnt
void Solution::pairing(vector<string>& words, int maxWidth, int startIndex)
{
    int spaceRemaining; // How many space is still available

    // Reset
    wordsCnt = 0;
    charCnt = 0;
    emptyCnt = 0;
    spaceRemaining = maxWidth;

    // When space available to add a further word &&
    // Not reach the end of the words
    while(((int)(words[startIndex + wordsCnt].size())) <= spaceRemaining)
    {
        charCnt += words[startIndex + wordsCnt].size();
        wordsCnt++;

        // Space remaining for further words
        // Add an empty space after each word
        spaceRemaining = maxWidth - charCnt - wordsCnt;

        // Reach the end of the words
        if ((startIndex + wordsCnt) >= (int)(words.size())) break;
    }

    emptyCnt = maxWidth - charCnt;
    
    return;
}


void Solution::centerJustify(vector<string>& words, int maxWidth, int startIndex)
{
     string str;
     int repeat;

     for(int i = 0; i < wordsCnt; i++)
     {
         // First get the average number of empty space
         // Then add an additional empty space
         // Example:
         // 4 words         -> wordsCnt = 4
         // 5 empty spaces  -> emptyCnt = 5
         //
         //                 -> average:                 emptyCnt / (wordsCnt-1) = 5 /(4 - 1) = 1
         //                 -> additional for i=0:      (5 % (4 - 1) > 0) = (2 > 0) = TRUE
         //                 -> additional for i=1:      (5 % (4 - 1) > 1) = (2 > 1) = TRUE
         //                 -> additional for i=2:      (5 % (4 - 1) > 1) = (2 > 2) = FALSE
         repeat = emptyCnt / (wordsCnt-1) + (emptyCnt % (wordsCnt-1) > i);

         // No empty space after last word
         if(i == (wordsCnt-1))
         {
             repeat = 0;
         }

         str.append(words[startIndex+i]);
         str.append(repeat, ' ');
     }

     res.push_back(str);

     return;
}


void Solution::leftJustify(vector<string>& words, int maxWidth, int startIndex)
{
     string str;
     int repeat = 1;

     for(int i = 0; i < wordsCnt; i++)
     {
         // Fill the empty space after the last word
         if(i == (wordsCnt-1))
         {
             repeat = maxWidth - charCnt - wordsCnt + 1;
         }

         str.append(words[startIndex+i]);
         str.append(repeat, ' ');
     }

     res.push_back(str);

     return;
}


vector<string> Solution::fullJustify(vector<string>& words, int maxWidth)
{
    // Reset
    int startIndex = 0;
    res.clear();

    while(1)
    {
        // Step 1: Find the words to build a line
        pairing(words, maxWidth, startIndex);

        // Step 2: Build the line
        if((startIndex+wordsCnt) < (int)(words.size()))
        {
            if(wordsCnt == 1)
            {
                // If the line has only one word, it is left justified
                leftJustify(words, maxWidth, startIndex);
            }
            else
            {
                centerJustify(words, maxWidth, startIndex);
            }
            startIndex += wordsCnt;
        }
        else
        {
            // Last line
            leftJustify(words, maxWidth, startIndex);
            break;
        }
    }
    
    return res;
}
\$\endgroup\$
3
  • 4
    \$\begingroup\$ Without a description of what the code is supposed to do we really can't tell how good the code is. Please add the description of the programming challenge from leetcode into the body of the question. The title of the post might be the name and number of the challenge on leetcode. Please read How do I ask a good question?. \$\endgroup\$
    – pacmaninbw
    May 9 at 12:26
  • \$\begingroup\$ Tried to look at the problem description using the link in the post… got an error saying that I had made too many requests. One request was too much, apparently. 🤷🏼 My opinion of LeetCode remains bottomed out. \$\endgroup\$
    – indi
    May 9 at 22:11
  • \$\begingroup\$ @pacmaninbw The fact that you can't tell what the code is supposed to do is one of the main issues with the code you should not need to refer to external sources to understand a small piece of code like this. \$\endgroup\$ May 10 at 9:59

1 Answer 1

1
\$\begingroup\$

I'm sorry, I know this is your first, of hopefully many, posts, but you asked if this is good coding, and my answer is sorry but I don't think it is. Before you get too upset, please let me explain, and remember I'm trying to help (but I what it feels like to get told your code isn't good, its happened to me many times)

I haven't clicked in the link, so I don't know what you are trying to do, because I shouldn't have to, your code should tell me all I need to know to understand your code. The class doesn't explain what its for and the name solution doesn't provide too many clues. The class members and attributes don't have comments explaining their purpose. In summary add meaningful comments.

Before you read the rest of the comments please be aware I don't 100% understand what the code does.

There is no clear differentiation between the class members, parameters and function variables which makes it harder to read, try m_wordsCount or similar.

while(1) This just makes it harder for the reader to understand what the condition that terminates to loop is. Have to be honest I'm still not sure what the difference between the leftJustify that terminates the loop and the other one is. break should really be a last resort (think of it as a goto) unless you are in a switch statement.

in centerJustify you set the value of repeat twice in certain circumstances, move the first assignment into an else and avoid that.

Why write Cnt? Does it make the code clearer and easier to read? Have tyo be honest I only inserted a u in the variable names when I read it and it didn't make sense. On the same subject single letter variables are a bad habit, some people say i is OK in loops, but in the case of centreJustify using word would have documented the code better.

Make arguments const ref unless you are changing their values. It states your intention, documents the code.

In pairing you are using a while loop. I think a for loop would have been better, but you really need to sort out the exit condition is it when the space runs out or the words do or both?

using namespace std; should be avoided, because it removes the purpose of namespaces.

A Good Point is that you have ensured that the sign of the values you are checking in pairing are the same, which is great, but looking at it from a different perspective why are wordCnt and spaceRemaining signed? Can you have a negative word count?

I have seen a code that is a lot worse so please don't take this too badly, you write clean well formatted code, I could read it even if I couldn't 100% understand it. You have a good style, and without trying to sound too patronizing, keep tweaking it as you go.

\$\endgroup\$
5
  • \$\begingroup\$ “Why write Cnt? Does it make the code clearer and easier to read? Have tyo be honest I only inserted a u in the variable names when I read it and it didn't make sense.” I know, right? I was sorely tempted to write a review, just to say something about the tragedy of emptyCnt. But for the record, I would say using signed values for all the (ahem) Cnt types is the correct thing to do. \$\endgroup\$
    – indi
    May 9 at 22:06
  • \$\begingroup\$ @indi Suggesting a u in this case may be inappropriate. I personally would prefer count. Not of fan of leetcode myself. \$\endgroup\$
    – pacmaninbw
    May 9 at 23:05
  • \$\begingroup\$ @indi - Being serious, why do you think counts should be signed? \$\endgroup\$ May 10 at 9:53
  • 1
    \$\begingroup\$ The question should not have been answered in its current state, it was clearly off-topic. \$\endgroup\$
    – pacmaninbw
    May 10 at 11:28
  • \$\begingroup\$ @CodeGorilla I think numbers, generally, should be signed. Unsigned types don’t follow the rules of normal arithmetic, they follow the rules of modular arithmetic; this is why over/underflowing an unsigned value isn’t UB… which means compilers have to account for it, which means less efficient code gen. You may argue that you could over/underflow a signed value, too, which would be UB, which would be bad… but to do so you have to pass (at least) ±32,767 (for int), which you’re a lot less likely to be close to than you are to 0. \$\endgroup\$
    – indi
    May 10 at 20:30

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