Bad variable naming (kSourceURLLength) for a variable that isn't a constant

NEW
Unassigned

Status

()

Core
Widget: Win32
--
minor
13 years ago
9 years ago

People

(Reporter: Daniel Atallah, Unassigned)

Tracking

Trunk
x86
Windows XP
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 1 obsolete attachment)

(Reporter)

Description

13 years ago
(17:07:09) timelyx: datallah: lastly, could you file a bug about kSourceURLLength
(17:07:13) timelyx: that variable is terribly named
(17:07:59) datallah: timelyx: the bug would be that it is misnamed?
(17:08:04) timelyx: yes
(17:08:26) timelyx: the 'k' is really wrong. dropping the k prefix and
lowercasing the next letter would be fine
(Reporter)

Comment 1

13 years ago
Created attachment 190459 [details] [diff] [review]
rename kSourceURLLength variable to sourceURLLength since it isn't a constant
Attachment #190459 - Flags: review?(timeless)

Comment 2

13 years ago
Comment on attachment 190459 [details] [diff] [review]
rename kSourceURLLength variable to sourceURLLength since it isn't a constant

>Index: nsDataObj.cpp
>@@ -1200,7 +1200,7 @@ nsDataObj :: BuildPlatformHTML ( const c
>-  const PRInt32 kSourceURLLength    = mSourceURL.Length();
this line should probably have one more space before the = sign since for some
reason the code is trying to keep them at the same position:
>+  const PRInt32 sourceURLLength    = mSourceURL.Length();
>   const PRInt32 kNumberLength       = strlen(numPlaceholder);
> 
>   const PRInt32 kTotalHeaderLen     = strlen(startHTMLPrefix) +
Attachment #190459 - Flags: superreview?(neil.parkwaycc.co.uk)
Attachment #190459 - Flags: review?(timeless)
Attachment #190459 - Flags: review+

Comment 3

13 years ago
Comment on attachment 190459 [details] [diff] [review]
rename kSourceURLLength variable to sourceURLLength since it isn't a constant

>-  const PRInt32 kSourceURLLength    = mSourceURL.Length();
>+  const PRInt32 sourceURLLength    = mSourceURL.Length();
>   const PRInt32 kNumberLength       = strlen(numPlaceholder);
> 
>   const PRInt32 kTotalHeaderLen     = strlen(startHTMLPrefix) +
Well if you're going to be picky then kTotalHeaderLen isn't a constant either,
and I don't see why you're still declaring it const. And all the strings should
be in arrays so you can use sizeof instead of strlen. Also you didn't keep the
= signs lined up.
(Reporter)

Comment 4

13 years ago
Created attachment 190713 [details] [diff] [review]
Implementation of Neil's suggestions (and more)

After spending a ton of time trying to understand the various string types, I
came to the conclusion that at the very least the usage in this method wasn't
consistent.

I made all the "static" strings actually static to the method and also
pre-calculated the the static part of the header length.  If there is a more
preferred way to do this, I'd love to know.
(Reporter)

Updated

13 years ago
Attachment #190459 - Attachment is obsolete: true
Attachment #190713 - Flags: review?(timeless)

Comment 5

13 years ago
Comment on attachment 190713 [details] [diff] [review]
Implementation of Neil's suggestions (and more)

>RCS file: /cvsroot/mozilla/widget/src/windows/nsDataObj.cpp,v
>diff -u -d -p -r1.68 nsDataObj.cpp

it seems like all these should be PRUint32 instead of PRInt32, no?

>+  static const PRInt32 kHeaderLen = sizeof(kStartHTMLPrefix) - 1 +
>+                                    sizeof(kEndHTMLPrefix) - 1 +
>+                                    sizeof(kStartFragPrefix) - 1 +
>+                                    sizeof(kEndFragPrefix) - 1 +
>+                                    sizeof(kEndFragTrailer) - 1 +
>+                                    (4 * (sizeof(kNumPlaceholder) - 1));
>+  PRInt32 totalHeaderLen          = kHeaderLen;
>+  PRInt32 startHTMLOffset = totalHeaderLen;
>   PRInt32 startFragOffset = startHTMLOffset
...
>   PRInt32 endFragOffset   = startFragOffset
...
>   PRInt32 endHTMLOffset   = endFragOffset
Attachment #190713 - Flags: superreview?(neil.parkwaycc.co.uk)
Attachment #190713 - Flags: review?(timeless)
Attachment #190713 - Flags: review+

Comment 6

13 years ago
Comment on attachment 190713 [details] [diff] [review]
Implementation of Neil's suggestions (and more)

>+  clipboardString.Append(kStartHTMLPrefix);
>   clipboardString.Append(nsPrintfCString("%08u", startHTMLOffset));
> 
>+  clipboardString.Append(kEndHTMLPrefix);
>   clipboardString.Append(nsPrintfCString("%08u", endHTMLOffset));
> 
>+  clipboardString.Append(kStartFragPrefix);
>   clipboardString.Append(nsPrintfCString("%08u", startFragOffset));
> 
>+  clipboardString.Append(kEndFragPrefix);
>   clipboardString.Append(nsPrintfCString("%08u", endFragOffset));
I'm wondering whether this couldn't be done in one big nsPrintfCString.

>+  clipboardString.Append(kEndFragTrailer);
> 
>+  clipboardString.Append(kHtmlHeaderString);
>+  clipboardString.Append(kFragmentHeaderString);
Hmm... I suppose you could switch to #define and use clipboardString.AppendLiteral(kEndFragTrailer kHtmlHederString kFragmentHeaderString); but I guess that's less readable.

>+  clipboardString.Append(kTrailingString);
I'd consider using AppendLiteral anyway, except that it's not documented to work on static const char arrays...
Attachment #190713 - Flags: superreview?(neil) → superreview+
Daniel, were you still planning on working on this?
(Reporter)

Comment 8

11 years ago
(In reply to comment #7)
> Daniel, were you still planning on working on this?
> 

I haven't had a working build setup for this in a while, so if someone else wants to take care of this that'd be great.
Assignee: win32 → nobody
QA Contact: ian → win32
You need to log in before you can comment on or make changes to this bug.