Could someone tell me how good/bad this code is and why, thanks [closed]

Question

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;
}

Answer 1

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.