Simple retrier of async function I wrote a simple function for retrying the passed function in case it throws.
I did it for my app that needs to call an external API through the network, which may spuriously fail.
export async function retry(fn, retries, delayMs, onFailedAttempt = null) {
    let lastError = null;

    for (let attempt = 0; attempt <= retries; attempt++) {
        if (attempt > 0) {
            await delay(delayMs);
        }

        try {
            const res = await fn();
            return res;
        } catch (error) {
            if (onFailedAttempt) {
                onFailedAttempt(error, attempt, retries, delayMs);
            }

            if (window.verbose) {
                console.debug(`Trial ${attempt + 1}/${retries} failed. Retrying in ${delayMs}ms...`);
            }

            lastError = error;
        }
    }

    throw lastError;
}

This seems to work, do you folks think I have overlooked any corner case?

Your code looks good and will work well. I have made some minor improvements to improve readability and reduce redundancy.
export async function retry(fn, retries, delayMs, onFailedAttempt) {
  let attempt = 0;

  while (true) {
    try {
      return await fn();
    } catch (error) {
      if (onFailedAttempt) {
        onFailedAttempt(error, attempt, retries, delayMs);
      }

      if (window.verbose) {
        console.debug(`Trial ${attempt + 1}/${retries} failed. Retrying in ${delayMs}ms...`);
      }

      if (attempt++ < retries) {
        await delay(delayMs);
      } else {
        throw error;
      }
    }
  }
}

Improvements:

onFailedAttempt = null in the function signature is redundant. If onFailedAttempt is not passed as an argument, its value will be undefined. Which will work equally well in your case when checking its value.

Moving await delay(delayMS) in catch block to keep related code together. As we only delay when an error occurs and if this was not the last attempt.

const res = await fn(), res is not needed here. You can simply do return await fn().

We could use the do...while here, but we'll have to check the attempt < retries condition two times in that situation. By using while (true), we only have to check it once.

We can also remove the lastError variable. If we get an error and this was the last attempt, we simply throw error.


Faster output to Sheet from a collection of Dictionaries My code is currently offloading parsed data into multiple spreadsheets, but it's pretty slow: about 110k rows and 35 columns in 3.5 minutes.
I was wondering if there were a more efficient way of looping through my collection of dictionaries.

Sub getData(Parsed As Object, company As String, a, wb)
        'Parsed contains a collection of dictionaries parsed from a JSON input'

        Dim oSheet As Variant
        Dim i As Long
        Dim j As Long
        For Each oSheet In ActiveWorkbook.Sheets
            'Identify the sheet containing the name of the company'
            Dim matchSheet As String
            If InStr(UCase(oSheet.Name), UCase(company)) Then
                matchSheet = oSheet.Name
            Else
                GoTo nextIteration
            End If
                'Create a 2d array of i number of dictionaries and j number of element in each dictionary'
                Dim Values As Variant
                ReDim Values(Parsed.Count, ArrayLen(a))

                Dim Value As Dictionary
                Dim key As Variant

                i = 1
                j = 0
                wb.Sheets(matchSheet).Select 'macro not working without selecting the sheet'
                wb.Sheets(matchSheet).Cells.Clear 'Clearing the data in the company Sheet'

                'For loops populating the Values array'
                For Each Value In Parsed
                    For Each key In a
                        Values(i, j) = Value(key)
                        j = j + 1
                    Next key
                    j = 0
                  i = i + 1
                Next Value

                'populating the needed sheet range with the parsed data'
                wb.Sheets(matchSheet).Range(Cells(1, 1), Cells(Parsed.Count, ArrayLen(a))) = Values
                'populationg the headers for that data'
                wb.Sheets(matchSheet).Range(Cells(1, 1), Cells(1, ArrayLen(a))).Value = a
                wb.Sheets(matchSheet).Range("D:D").NumberFormat = "General"
nextIteration:
        Next oSheet

End Sub

Here is an exemple of  what my API request returning 2 deals with 3 keys out of 37:

[
   {
      "DealNo": "11111",
      "DealDate": "2010-01-01",
      "Quantity": "1000"
   },
   {
      "DealNo": "11112",
      "DealDate": "2010-01-02",
      "Quantity": "2000"
   }
]

And here is the structure of the data after it has been parsed in excel:

A couple of review points.

Goto. Using Goto is a real bad smell. In your case, you can remove the spaghetti very easily.

Sub getData(Parsed As Object, company As String, a, wb)
        'Parsed contains a collection of dictionaries parsed from a JSON input'
        Dim oSheet As Variant
        Dim i As Long
        Dim j As Long
        For Each oSheet In ActiveWorkbook.Sheets
            'Identify the sheet containing the name of the company'
            Dim matchSheet As String
            If InStr(UCase(oSheet.Name), UCase(company)) Then
                matchSheet = oSheet.Name
            Else
                GoTo nextIteration
            End If
                ' [Main Code Block]
nextIteration:
        Next oSheet
End Sub

Becomes

Sub getData(Parsed As Object, company As String, a, wb)
        'Parsed contains a collection of dictionaries parsed from a JSON input'
        Dim oSheet As Variant
        Dim i As Long
        Dim j As Long
        For Each oSheet In ActiveWorkbook.Sheets
            'Identify the sheet containing the name of the company'
            Dim matchSheet As String
            If InStr(UCase(oSheet.Name), UCase(company)) Then
                matchSheet = oSheet.Name
                ' [Main Code Block]
            End If
        Next oSheet
End Sub

Much cleaner!

Select. I can see why your code does not work without using Select. And you do not need it. You have unqualified references to Range objects (in this case Cells) that are acting on the active sheet. By selecting the sheet, you are changing the active sheet but there is no guarantee that it is going to stay that way!

            wb.Sheets(matchSheet).Range(Cells(1, 1), Cells(Parsed.Count, ArrayLen(a))) = Values
            'populationg the headers for that data'
            wb.Sheets(matchSheet).Range(Cells(1, 1), Cells(1, ArrayLen(a))).Value = a
            wb.Sheets(matchSheet).Range("D:D").NumberFormat = "General"

Is better expressed in a With statement for brevity and ease of reading (note that the "." are also in front of the Cells, this is the critical change to your original code).

            With wb.Sheets(matchSheet)
                .Range(.Cells(1, 1), .Cells(Parsed.Count, ArrayLen(a))) = Values
                'populating the headers for that data'
                .Range(.Cells(1, 1), .Cells(1, ArrayLen(a))).Value = a
                .Range("D:D").NumberFormat = "General"
            End With

ReDim. Probably not a huge issue, but you ReDim every loop instance on values that are passed in at the start. Set your Values array up once before you enter the loop. By the looks of your code, you clobber the values in the array each time you go through the loop, so you don't even have to clear it.

I would guess that your biggest performance hit is iterating through the dictionaries. If you could bring your data in in a cleaner fashion, it may help your performance.


Why is this RC4 code in Racket so slow? According to shootout.alioth.debian.org, Racket is not much slower than C#, at least in the same order of magnitude due to its JIT compiler.

However, I'm seeing at least two degrees of magnitude slowdown in my implementation of the RC4 cipher in C# vs. Racket.

(Disclaimer: I am not using RC4, in this case a hilariously puny 16-bit version, to ensure any type of confidentiality. It is only used as a fast traffic obfuscating layer - that's why I need it to be fast!)

My (Typed) Racket code is here: https://github.com/quantum1423/Kirisurf-Official/blob/master/src/arcfour.rkt

The relevant portion:

;Returns a stateful function which encrypts one byte of data with the given key
(define (make-rc4 keyd)

  (define key keyd)

  (define S
     (make-bytes 256 0))
  (for ([i 256])
    (bytes-set! S i i))
  (define j 0)
  (for ([i 256])
    (set! j
          (unsafe-fxmodulo
           (unsafe-fx+ j
                       (unsafe-fx+ (bytes-ref S i)
                                   (bytes-ref key (unsafe-fxmodulo i (unsafe-bytes-length key)))))
         256))
    (define tmp (unsafe-bytes-ref S i))
    (unsafe-bytes-set! S i (bytes-ref S j))
    (unsafe-bytes-set! S j tmp))

  (set! j 0)
  (define i 0)

  (: Sr (Integer -> Integer))
  (define (Sr x) (unsafe-bytes-ref S x))

  (: toret (Integer -> Integer))
  (define (toret c)
    (set! i (unsafe-fxmodulo (unsafe-fx+ 1 i) 256))
    (set! j (unsafe-fxmodulo (unsafe-fx+ j (Sr i)) 256))
    (define temp (Sr i))
    (unsafe-bytes-set! S i (Sr j))
    (unsafe-bytes-set! S j temp)
    (bitwise-xor (Sr (unsafe-fxmodulo (unsafe-fx+ (Sr i) (Sr j)) 256)) c))
  toret)

My old C# code is here: https://github.com/quantum1423/Kirisurf-Official/blob/a04136569025f046872c3cf7fd07955f60bf49dc/Tunnel.cs (scroll to line 398 for the RC4State object)

To put things in perspective, my C# code is always I/O-bound and uses imperceptible CPU. My Racket code vrooms my CPU full and pulls an incredibly slow 700 KiB each second.

Here are some tips that may help. Note that these are collected from feedback on #racket on irc.freenode.net (which you may find helpful too).

The length of the bytestring is counted inside the loop, but it's a loop invariant that you can hoist out of the loop.
Folding over j may be faster than mutating it as you are doing. See Mutation and Performance in the Guide for more details.
Your for loops use generic sequences. Using specialized sequences like in-range would be faster.

You might also benefit from trying the Optimization Coach plugin for DrRacket. It's a tool that identifies performance pitfalls in your program along with possible solutions. For Typed Racket, it can also find when the type-driven optimizer was not able to optimize the program but could potentially do so with some changes.

Also try the Racket mailing list: http://www.racket-lang.org/community.html


Check if two rectangles overlap Suppose rectangles are parallel to x-axis/y-axis. Check if two rectangles overlap or not and if they do, output the overlap area.

Here is my code and I track min/max x-coordinate and min/max y-coordinate for each rectangle. Using Python 2.7.

Any comments on code bugs, code style and performance improvements in terms of algorithm time complexity are appreciated.

class Rectangle:
    def __init__(self, min_x, max_x, min_y, max_y):
        self.min_x = min_x
        self.max_x = max_x
        self.min_y = min_y
        self.max_y = max_y

    def is_intersect(self, other):
        if self.min_x > other.max_x or self.max_x < other.min_x:
            return False
        if self.min_y > other.max_y or self.max_y < other.min_y:
            return False
        return True

    def get_insersec_region(self, other):
        if not self.is_intersect(other):
            return None
        min_x = max(self.min_x, other.min_x)
        max_x = min(self.max_x, other.max_x)
        min_y = max(self.min_y, other.min_y)
        max_y = min(self.max_y, other.max_y)

        return Rectangle(min_x, max_x, min_y, max_y)

    def __str__(self):
        return str(self.min_x) + '\t' + str(self.max_x) + '\t' + str(self.min_y) + '\t' + str(self.max_y)

if __name__ == "__main__":
    r1 = Rectangle(0,10,0,10)
    r2 = Rectangle(5,15,5,15)
    print r1.is_intersect(r2)
    print r1.get_insersec_region(r2)

Instead of get_insersec_region, I would call this method __and__. This way you can use overlap = r1 & r2, just like for set intersection.

You can add intersect = __and__ if you also want the expressiveness of overlap = r1.intersect(r2).

Instead of returning None, I would consider returning an empty Rectangle. This way you don't need any special code to handle e.g. (r1&r2).area. For this, I would set default values for the coordinates, all 0.

Incidentally, I would add an area method and make it a property:

@property
def area(self):
    return (self.max_x - self.min_x) * (self.max_y - self.min_y)

The reason it should be a property is maintainability. If you decide at some point that you only want to compute the area at creation time because a Rectangle is not allowed to change size anymore, you could remove this function and just store self.area = ... in the constructor. Also it is evidently true that the area of a rectangle is a property of that particular rectangle.

And an __or__ method to get the bounding rectangle. You might need this if you want a GUI and update only the region where things changed but still want to do only one update. Then you need to find a rectangular area which includes all objects, which you can then just get by |ing all rectangles.

Your __str__ method can be simplified using map and str.join:

def __str__(self):
    return '\t'.join(map(str, (self.min_x, self.max_x, self.min_y, self.max_y)))

Alternatively, you can use str.format:

def __str__(self):
    return '{self.min_x}\t{self.max_x}\t{self.min_y}\t{self.max_y}'.format(self=self)

The latter slightly more future proof, because you can replace it with this in Python 3.6, using f-strings:

def __str__(self):
    return f'{self.min_x}\t{self.max_x}\t{self.min_y}\t{self.max_y}'

Final code:

class Rectangle:
    def __init__(self, min_x=0, max_x=0, min_y=0, max_y=0):
        self.min_x = min_x
        self.max_x = max_x
        self.min_y = min_y
        self.max_y = max_y

    def is_intersect(self, other):
        if self.min_x > other.max_x or self.max_x < other.min_x:
            return False
        if self.min_y > other.max_y or self.max_y < other.min_y:
            return False
        return True

    def __and__(self, other):
        if not self.is_intersect(other):
            return Rectangle()
        min_x = max(self.min_x, other.min_x)
        max_x = min(self.max_x, other.max_x)
        min_y = max(self.min_y, other.min_y)
        max_y = min(self.max_y, other.max_y)
        return Rectangle(min_x, max_x, min_y, max_y)

    intersect = __and__

    def __or__(self, other):
        min_x = min(self.min_x, other.min_x)
        max_x = max(self.max_x, other.max_x)
        min_y = min(self.min_y, other.min_y)
        max_y = max(self.max_y, other.max_y)
        return Rectangle(min_x, max_x, min_y, max_y)

    union = __or__

    def __str__(self):
        return 'Rectangle({self.min_x},{self.max_x},{self.min_y},{self.max_y})'.format(self=self)

    @property
    def area(self):
        return (self.max_x - self.min_x) * (self.max_y - self.min_y)


Find highly composite number using Python 3 A highly composite number (HCN) is a positive integer with more divisors than any smaller positive integer ha according to Wikipedia. Here is a YouTube video about it.

def get_divisors(number_range):
  divisor_list = []
  divisor_list2 = []
  for number in range(1,number_range+1):
    for e in range(1,number_range+1):
      if number % e == 0:
        divisor_list2.append(e)
    divisor_list.append(divisor_list2)
    divisor_list2 = []
  return divisor_list

def get_highly_composite_number(number_range):
  divisor_list = get_divisors(number_range)
  highly_composite_number_list = []
  number_of_divisor_list = []
  number_of_divisor_list2 = []
  number_of_divisor_list3 = []
  tracker = 0
  for e in divisor_list:
    number_of_divisor_list.append(len(e))
    number_of_divisor_list2.append(max(number_of_divisor_list))
  for e in number_of_divisor_list2:
    number_of_divisor_list3.append(e)
    if number_of_divisor_list3[tracker] == max(number_of_divisor_list3) and number_of_divisor_list3.count(number_of_divisor_list3[tracker]) == 1:
      highly_composite_number_list.append(tracker+1)
      tracker += 1
    else:
      tracker += 1
  return highly_composite_number_list

I know the naming is horrible, but I did not know any better ways. Also, I know this can probably be condensed, but I did not want to touch it.

As you said, the naming is not good and the code can be condensed and optimized.

You don't need to keep all the lists, you just need the count
You can split get_divisors so you have another function to get the divisors of a single number
You can use a dictionary to store the divisor count of the numbers.

This would already take care of the variable names:

def get_divisors_count(number):
  divisors = []
  for divisor in range(1, number + 1):
    if (number % divisor) == 0:
      divisors.append(divisor)
  return len(divisors)

def get_divisors_range(number_range):
  divisor_count = {}
  for number in range(1, number_range):
    divisor_count[number] = get_divisors_count(number)
  return divisor_count

def get_highly_composite_number(number_range):
  divisor_list = get_divisors_range(number_range)
  highly_composite_number_list = []
  for i in range(number_range - 1, 0, -1):
    found = False
    for j in range(1, i):
      if (divisor_list[j] >= divisor_list[i]):
        found = True
        break
    if (not found):
      #If you want the list sorted from low to high
      #change this to highly_composite_number_list.insert(0, i)
      highly_composite_number_list.append(i)
  return highly_composite_number_list

This could also be optimized a bit using a lookup for numbers of which you already computed the divisors. For examples, 4 is a divisor of 8, but when you check 8, you can lookup the divisors of 4 and use that.


Implementation of Parallel.ForEach for .NET 3.5 Because the TPL backport not only contains Parallel.* but also PLINQ, concurrent collections, etc, I will try to use this one.

Just for fun, I profiled both the TPL implementation and my own, in the very basic and naive test, mine was actually a little bit faster.

Microsoft TPL backport: 00:00:17.0673098
My Implementation: 00:00:16.9210535
Intel Q6600 (quad-core), 4GB of RAM, Win8.1(x64), VS2013 targetting .NET 3.5

The code is here (github gist), the colorful output of the Concurrency Visualizer is below:

This negligeable performance difference (in the best case) gets outweight by the many optimisations the .NET team put in for special cases.

Question:

To ease working with multithreaded code, I wanted to implement a version of Parallel.ForEach for .NET 3.5.

We are for the moment stuck with 3.5 and cannot upgrade.  We cannot use external libraries like TPL.

I found this via Google, but this threw sporadic exceptions during runtime, and because I couldn't find another Version I decided to roll my own.

I did of course test it, but i am always a bit cautious with multithreaded code.

public class Parallel
{
    public static void ForEach<T>(IEnumerable<T> items, Action<T> action)
    {
        if (items == null)
            throw new ArgumentNullException("enumerable");
        if (action == null)
            throw new ArgumentNullException("action");

        var resetEvents = new List<ManualResetEvent>();

        foreach (var item in items)
        {
            var evt = new ManualResetEvent(false);
            ThreadPool.QueueUserWorkItem((i) =>
            {
                action((T)i);
                evt.Set();
            }, item);
            resetEvents.Add(evt);
        }

        foreach (var re in resetEvents)
            re.WaitOne();
    }
}

if (items == null)
        throw new ArgumentNullException("enumerable");

In the execution path where this exception is thrown, the reported argument name will not match the actual argument name. If this is the result of a rename refactoring (i.e. it's the parameter formerly known as "enumerable"), it's unfortunate, the string literal wasn't taken care of. That doesn't happen with ReShaper performing the refactoring, but if you're using the refactoring built into VS, I'm not sure it handles string literals; if you've manually "renamed" the argument, ...next time, use refactor/rename! ;)

Your implementation queues a work item for each item in items.

  ThreadPool.QueueUserWorkItem

  Queues a method for execution. The method executes when a thread pool thread becomes available.

The interesting bit is about its return value:

  Return Value

  Type: System.Boolean

  true if the method is successfully
  queued; NotSupportedException is thrown if the work item could not be
  queued.

There's the slight possibility that thread pooling might not be supported on the platform code is running on (from what I gathered of reasons for this exception to be thrown), in which case the first iteration will throw a NotSupportedException that the ForEach code might not be expecting at all... but letting it bubble up might just be the best thing to do in this case.

If I correctly understand what's going on, you're essentially spawning a thread to run an action, for each item in items. This might not be as efficient as you'd think it is: there's only so many concurrent operations that can happen on a quad-core processor, and then there's overhead too; sometimes it's more efficient to simply run the tasks sequentially.

And that's what Parallel.ForEach does:

  Parallel.ForEach

  Executes a foreach operation on an IEnumerable in which iterations may run in parallel.

Thus, I'd use your implementation with care, and perhaps use a profiler and benchmark against a "normal" foreach for a given number of iterations, but then your dev environment might have different computing power than your test and/or production environments, so careful with benchmarks, too.

It might be simpler to use the Microsoft NuGet package @JesseC.Slicer and @PhilBolduc mentioned in the comments...


Extracting all the data from a table out of a javascript encrypted website I've written a script to harvest all the data out of a table from a webpage using python in combination with selenium. It takes a while to parse them all. There are seven steps to hurdle to get to the target page. The search criterion for the table is "pump". However, when the table shows up, there is an option button to select "ALL" appearing in the downmost portion. After selecting the "All" from the options, the site then displays the data with full table. This script is able to automate the whole procedure. I tried to make my code faster using explicit wait maintaining the guidelines of selenium. It is doing it's job perfectly now. Here is the working code.

from selenium import webdriver
from selenium.webdriver.common.by import By
from selenium.webdriver.support.wait import WebDriverWait
from selenium.webdriver.support import expected_conditions as EC

driver = webdriver.Chrome()
wait = WebDriverWait(driver, 10)
driver.get('http://apps.tga.gov.au/Prod/devices/daen-entry.aspx')

driver.find_element_by_id('disclaimer-accept').click()

wait.until(EC.visibility_of_element_located((By.ID, "medicine-name")))
driver.find_element_by_id('medicine-name').send_keys('pump')

wait.until(EC.visibility_of_element_located((By.ID, "medicines-header-text")))
driver.find_element_by_id('medicines-header-text').click()
driver.find_element_by_id('submit-button').click()

wait.until(EC.visibility_of_element_located((By.ID, "ctl00_body_MedicineSummaryControl_cmbPageSelection")))
driver.find_element_by_id("ctl00_body_MedicineSummaryControl_cmbPageSelection").click()
driver.find_element_by_xpath('//option[@value="all"]').click()

wait.until(EC.visibility_of_element_located((By.ID, "ctl00_body_MedicineSummaryControl_grdSummary")))
tab_data = driver.find_element_by_id("ctl00_body_MedicineSummaryControl_grdSummary")

list_rows = []
for items in tab_data.find_elements_by_xpath('.//tr'):
    list_cells = []
    for item in items.find_elements_by_xpath('.//td[@class="row-odd"]|.//td'):
        list_cells.append(item.text)
    list_rows.append(list_cells)
for data in list_rows:
    print(data)

driver.quit()

Your code is getting better from script to script and there is less things to point out. I would improve a couple of things only:

the way you get the data - I think you can use the "by tag name" locators with a nested list comprehension:

list_rows = [[cell.text for cell in row.find_elements_by_tag_name('td')]
             for row in tab_data.find_elements_by_tag_name('tr')]

you can use a Select class to select an option from a select dropdown:

from selenium.webdriver.support.select import Select

results_count = Select(driver.find_element_by_id("ctl00_body_MedicineSummaryControl_cmbPageSelection"))
results_count.select_by_visible_text("All")


Maths quiz with difficulty levels I've written this code but it's really long. I really need it to be shorter so can someone please help me! Any suggestions will be helpful.

choice=raw_input("What type of quiz do you want to do: maths or science? ")
topic=open("topic.txt", "a+")
topic.write(choice + '\n')
topic.close()
difficulty=raw_input("What difficulty do you want to play on: easy, medium or hard? ")
diff=open("difficulty.txt", "a+")
diff.write(difficulty + '\n')
diff.close()

score = 0

def answercheck(score, str_answer, int_answer):
    user_answer=raw_input("Select your answer. ")
    if user_answer.upper() == str(str_answer) or user_answer.upper() == str(int_answer):
        print("Well done, that's correct")
        score+=1
        print("Your score is " +str(score))
    else:
        print ("Wrong, it was "+str(int_answer))
        print("Your score is " +str(score))
    raw_input("Press enter to continue")
    return score

def report(score):
    print("Your final score is "+str(score))
    per=str(score)*(100/5)
    print("You achieved "+per+"%")
    if score==5:
        print("You achieved an A*")
    elif score==4:
        print("You acieved a grade B")
    elif score==3:
        print("You achieved a grade C")
    elif score==2:
        print("You achieved a grade D")
    elif score<=1:
        print("You failed")

if choice.lower() == "maths" and difficulty.lower() == "easy":
    easym=open("mathseasy.txt" , "r")
    lines = easym.readlines()
    print lines[0]
    print lines[1]
    print("A. 4"+'\n'+"B. 6")
    str_answer_one="A"
    int_answer_one=4
    first_score=answercheck(score, str_answer_one, int_answer_one)

    print lines[2]
    print("A. 5"+'\n'+"B. 6")
    str_answer_two="A"
    int_answer_two=5
    second_score=answercheck(first_score, str_answer_two, int_answer_two)

    print lines[3]
    print("A. 15"+'\n'+"B. 20")
    str_answer_three="B"
    int_answer_three=20
    third_score=answercheck(second_score, str_answer_three, int_answer_three)

    print lines[4]
    print("A. 13"+'\n'+"B. 15")
    str_answer_four="A"
    int_answer_four=13
    fourth_score=answercheck(third_score, str_answer_four, int_answer_four)

    print lines[5]
    print("A. 100"+'\n'+"B. 110")
    str_answer_five="B"
    int_answer_five=110
    fifth_score=answercheck(fourth_score, str_answer_five, int_answer_five)
    report(fifth_score)

if choice.lower() == "maths" and difficulty.lower() == "medium":
    mediumm=open("mathsmedium.txt" , "r")
    lines = mediumm.readlines()
    print lines[0]
    print lines[1]
    print("A. 30"+'\n'+"B. 35")
    str_answer_one="A"
    int_answer_one=30
    first_score=answercheck(score, str_answer_one, int_answer_one)

    print lines[2]
    print("A. 100"+'\n'+"B. 110")
    str_answer_two="B"
    int_answer_two=110
    second_score=answercheck(first_score, str_answer_two, int_answer_two)

    print lines[3]
    print("A. 13"+'\n'+"B. 15")
    str_answer_three="A"
    int_answer_three=13
    third_score=answercheck(second_score, str_answer_three, int_answer_three)

    print lines[4]
    print("A. 30"+'\n'+"B. 32")
    str_answer_four="B"
    int_answer_four=32
    fourth_score=answercheck(third_score, str_answer_four, int_answer_four)

    print lines[5]
    print("A. 21"+'\n'+"B. 29")
    str_answer_five="B"
    int_answer_five=29
    fifth_score=answercheck(fourth_score, str_answer_five, int_answer_five)
    report(fifth_score)

Here is the data in mathseasy.txt:

Welcome to the easy maths quiz.
What's 2+2?
What's 11-6?
What's 5*4?
What's 26/2?
What's 11*10?

Here is the data in mathsmedium.txt:

Welcome to the medium maths quiz.
What's 5*6?
What's 79+31?
What's 26/2?
What's 4*8?
What's 50-21?

You could try making the questions/answers declarative and then the logic surrounding the questions/answers could then be factored out. An easy way would be to store the questions/answers in a list of tuples:

data = [("What is 5+5?", 10), ("What is 10+10?", 20) ...]


Menu selector that changes the background of a selected div I made this code to change the background color for a div in a set of divs while disabling the rest of the divs in the background. It works fine, but I wanted the code reviewed for more improvements since I am new to jQuery and JavaScript.

عن المشروع

المميزات الرئيسية

الخارطة

البوم الصور والفيديو

  var count = $(".ProjectMainLink").length;
     for(var i=0; i<count;i++){
         $(".ProjectMainLink").eq(i).click(function() {
              $(".ProjectMainLink").css("background-color","");
            $(this).css("background-color","#fff");

        });
         }

im not sure why you would want the length or count of the class..
You could simply use this to highlight the divs:

    $(".ProjectMainLink").click(function() {
          $(".ProjectMainLink").css("background-color","");
          $(this).css("background-color","#000");

    });

JS FIDDLE: http://jsfiddle.net/p3m2p/

EDIT:

Don't forget to wrap your functions in <script type="text/javascript"></script>

It could also be done like this:

<script type="text/javascript">
$(document).on("click",".ProjectMainLink",function() {
          $(".ProjectMainLink").css("background-color","");
          $(this).css("background-color","#000");
    });
</script>


SQL database design for ecommerce I need to create a database which stores details products, Manufactures,Suppliers. So initially I divided Products into categories and subcategories.

These are the tables I created.

Manufacturer, Category, Sub_Category, Products,
ProductSpecs, ProductAvailability, Supplier

Manufacturer table contains Brand Details (Ex: Apple,Windows..etc).
Supplier table should contain supplier details and price they are offering.
Category contains provides details like Electronics, Sports...etc.
Subcategory table should contains subcategories like Electronics(Mobiles,Laptops...).
Products table have only basic product details like ID,Name.
ProductSpecs contains specifications. Based on product we should have separate separate table for each subcategory.
ProductAvailability should contains product price and availability.

I am not good at database design. But managed to create the below one. Could you please anyone review this? Please help me if it needs any corrections.

CREATE TABLE Supplier
  (
  SupplierID INT IDENTITY(1,1) NOT NULL PRIMARY KEY,
  SupplierName VARCHAR(30) NOT NULL DEFAULT '',
  SupplierTag  VARCHAR(30) NOT Null DEFAULT '',
  );

CREATE TABLE Category
    (
    CategoryID INT IDENTITY(1,1) NOT NULL PRIMARY KEY,
    CategoryName VARCHAR(30) NOT NULL DEFAULT '',
    CreatedBy VARCHAR(30) NOT NULL DEFAULT '',
    CreatedOn DATETIME NOT NULL
    );

CREATE TABLE Sub_Category
    (
    SubCategoryID INT IDENTITY(1,1) NOT NULL PRIMARY KEY,
    SubCategoryName VARCHAR(30) NOT NULL,
    CategoryID INT FOREIGN KEY REFERENCES CATEGORY(CategoryID),
    CreatedBy VARCHAR(30) NOT NULL,
    CreatedOn DATETIME NOT NULL
    );

CREATE TABLE Manufacturer
    (
    ManufacturerID INT IDENTITY(1,1) NOT NULL PRIMARY KEY,
    ManufacturerName VARCHAR(30) NOT NULL DEFAULT '',
    SubCategoryID INT FOREIGN KEY REFERENCES SUB_CATEGORY(SubCategoryID),
    );

CREATE TABLE Products
  (
  ProductID INT IDENTITY(1,1) NOT NULL PRIMARY KEY,
  ProductName   VARCHAR(50) NOT NULL DEFAULT '',
  CategoryID INT FOREIGN KEY REFERENCES CATEGORY(CategoryID),
  SubCategoryID INT FOREIGN KEY REFERENCES SUB_CATEGORY(SubCategoryID),
  ManufacturerID INT FOREIGN KEY REFERENCES Manufacturer(ManufacturerID)
  );

CREATE TABLE ProductSpecs
(
  RowID INT IDENTITY(1,1) NOT NULL PRIMARY KEY,
  ProductID INT FOREIGN KEY REFERENCES Products(ProductID),
  SubCategoryID INT FOREIGN KEY REFERENCES SUB_CATEGORY(SubCategoryID),
  ProductSpec1 VARCHAR(30),
  ProductSpec2 VARCHAR(30),
  ProductSpec3 VARCHAR(30),
)

CREATE TABLE ProductAvailability
(
  RowID INT IDENTITY(1,1) NOT NULL PRIMARY KEY,
  ProductID INT FOREIGN KEY REFERENCES Products(ProductID),
  SupplierID INT FOREIGN KEY REFERENCES Supplier(SupplierID),
  SupplierWEB Varchar(30) NOT NULL,
  ProductPrice decimal(6,2) NOT NULL,
)

CREATE TABLE Supplier
  (
  SupplierID INT IDENTITY(1,1) NOT NULL PRIMARY KEY,
  SupplierName VARCHAR(30) NOT NULL DEFAULT '',
  SupplierTag  VARCHAR(30) NOT Null DEFAULT '',
  );

Why limit the name to thirty characters?  Consider names like "Specialty Products Company of Transylvania" -- that's forty-two characters.  Unless you have a reason to keep it short, it's better to pick a larger width than necessary rather than smaller.  A VARCHAR(255) with a thirty character string in it is the same length as if it were a VARCHAR(30).  You only use storage for what you use.

Also consider using NVARCHAR instead of VARCHAR.  Perhaps some of your suppliers will have names that require Unicode to represent.

CREATE TABLE Category
    (
    CategoryID INT IDENTITY(1,1) NOT NULL PRIMARY KEY,
    CategoryName VARCHAR(30) NOT NULL DEFAULT '',
    CreatedBy VARCHAR(30) NOT NULL DEFAULT '',
    CreatedOn DATETIME NOT NULL
    );

CREATE TABLE Sub_Category
    (
    SubCategoryID INT IDENTITY(1,1) NOT NULL PRIMARY KEY,
    SubCategoryName VARCHAR(30) NOT NULL,
    CategoryID INT FOREIGN KEY REFERENCES CATEGORY(CategoryID),
    CreatedBy VARCHAR(30) NOT NULL,
    CreatedOn DATETIME NOT NULL
    );

You can only have two levels of category and you must have two levels of category.  If you want to add a third level, you'd have to add a new table (or refactor the existing tables).  It is more common to handle this with a single table that has a ParentID column.

CREATE TABLE Category
    (
    CategoryID INT IDENTITY(1,1) NOT NULL PRIMARY KEY,
    ParentID INT NOT NULL DEFAULT 0,
    CategoryName VARCHAR(30) NOT NULL DEFAULT '',
    CreatedBy VARCHAR(30) NOT NULL DEFAULT '',
    CreatedOn DATETIME NOT NULL
    );

Example showing how to use a table like this:  Getting all the children of a parent using MSSQL query.

CREATE TABLE Manufacturer
    (
    ManufacturerID INT IDENTITY(1,1) NOT NULL PRIMARY KEY,
    ManufacturerName VARCHAR(30) NOT NULL DEFAULT '',
    SubCategoryID INT FOREIGN KEY REFERENCES SUB_CATEGORY(SubCategoryID),
    );

This says that a Manufacturer is associated with a SUB_CATEGORY.  This seems overly restrictive.  For example what if your subcategories are video and music?  A company may very well deal in both.

CREATE TABLE Products
  (
  ProductID INT IDENTITY(1,1) NOT NULL PRIMARY KEY,
  ProductName   VARCHAR(50) NOT NULL DEFAULT '',
  CategoryID INT FOREIGN KEY REFERENCES CATEGORY(CategoryID),
  SubCategoryID INT FOREIGN KEY REFERENCES SUB_CATEGORY(SubCategoryID),
  ManufacturerID INT FOREIGN KEY REFERENCES Manufacturer(ManufacturerID)
  );

Given your original schema, you should be careful about having both a CategoryID and a SubCategoryID.  This is what is called a denormalized schema.  You can associate a CategoryID with a Product in two different ways.  In a normalized schema, you only want there to be one association.  Consider the case where CategoryID is not the parent of SubCategoryID.

Note that if you change to a single table with a ParentID, that it will also make sense to only associate the product with one level of category.

Consider the possibility that you might need products to be in more than one category at once.  For example, you might create a Specials category.  In that case, you might need to link Products and Category with a table rather than a column.

Note that you usually either want all the names to be singular (like Category and Manufacturer) or all to be plural (like Products).  Pick a convention and stick to it.

CREATE TABLE ProductSpecs
(
  RowID INT IDENTITY(1,1) NOT NULL PRIMARY KEY,
  ProductID INT FOREIGN KEY REFERENCES Products(ProductID),
  SubCategoryID INT FOREIGN KEY REFERENCES SUB_CATEGORY(SubCategoryID),
  ProductSpec1 VARCHAR(30),
  ProductSpec2 VARCHAR(30),
  ProductSpec3 VARCHAR(30),
)

Given this description, you might be better off adding three columns to Products.  Although I would strong consider making a Specification table and a SpecificationToProduct table.  That way you can easily add more than three specifications to a product and multiple products can have the same specification.

Try to avoid names like RowID.  Go ahead and name this ProductSpecID or leave off that column altogether.

CREATE TABLE ProductAvailability
(
  RowID INT IDENTITY(1,1) NOT NULL PRIMARY KEY,
  ProductID INT FOREIGN KEY REFERENCES Products(ProductID),
  SupplierID INT FOREIGN KEY REFERENCES Supplier(SupplierID),
  SupplierWEB Varchar(30) NOT NULL,
  ProductPrice decimal(6,2) NOT NULL,
)

This seems a good place to use a composite primary key based on ProductID and SupplierID rather than RowID.

CREATE TABLE ProductAvailability
(
  ProductID INT FOREIGN KEY REFERENCES Products(ProductID),
  SupplierID INT FOREIGN KEY REFERENCES Supplier(SupplierID),
  SupplierWEB Varchar(30) NOT NULL,
  SupplierPrice decimal(6,2) NOT NULL,
  PRIMARY KEY (ProductID, SupplierID)
)

If ProductPrice is the supplier's price for the product, then SupplierPrice might be a better name.

It's not clear to me what a SupplierWEB is nor why it would be associated with the Supplier/Product link.  If it is the supplier's product page, then a name change might be clearer.  Also, it should probably be wider than thirty characters if it is meant to hold a link.

Your diagram does not match your table specifications.  I reviewed the table specifications.  If the diagram better reflects your actual design, you should post another question where the table specifications match the diagram.


Sorting function that also works with containers having only forward iterators I was playing around with some code trying to implement a sorting algorithm in C++ that also works with singly linked lists. Since the merge routine of mergesort only scans forward, implementing a generic mergesort seemed like the way to go.

In C, I would have implemented it as follows:

typedef struct node {
  int val;
  struct node *next;
} node_t;

/* Merges two sorted linked lists placing the result in the
   first list. The function modifies both lists */
node_t* merge(node_t *ha, node_t *hb)
{
  if(!ha)
    return hb;
  if(!hb)
    return ha;

  node_t *i;
  node_t *prev;
  node_t *j = hb;

  prev = NULL;
  i = ha;
  while(i || j) {
    if(!j) break;
    if(!i) {
      prev->next = j;
      break;
    }

    if(j->val < i->val) {
      if(!prev)
        ha = j;
      else
        prev->next = j;

      prev = j;
      j = j->next;
      prev->next = i;
    }
    else {
      prev = i;
      i = i->next;
    }
  }

  return ha;
}

node_t* mergesort(node_t *ha)
{
  if(!ha || !ha->next)
    return ha;

  node_t * prev;
  node_t *mid = ha;
  node_t *fast = ha;

  /* Find midpoint of linked list */
  while(1) {
    fast = fast->next;
    if(!fast) break;
    fast = fast->next;
    if(!fast) break;
    mid = mid->next;
  }

  prev = mid;
  mid = mid->next;
  prev->next = NULL;
  ha = mergesort(ha);
  mid = mergesort(mid);
  return merge(ha, mid);
}

Here is my C++ implementation, which I would like to have reviewed:

#include <iostream>
#include <vector>
#include <iterator>
#include <algorithm>
#include <list>

template<typename InputIt1, typename InputIt2, typename OutputIt>
OutputIt my_merge(InputIt1 first1,
               InputIt1 end1,
               InputIt2 first2,
               InputIt2 end2,
               OutputIt out)
{
  while(first1 != end1) {
    if(first2 == end2)
      return std::copy(first1, end1, out);

    if(*first1 < *first2) {
      *out = *first1;
      ++first1;
    }
    else {
      *out = *first2;
      ++first2;
    }
    ++out;
  }

  return std::copy(first2, end2, out);
}

template<typename InputIt>
void mergesort(InputIt first, InputIt end)
{
  size_t n = std::distance(first, end);
  if(n < 2)
    return;
  InputIt mid = first;

  for(size_t i = 0; i < n/2; ++i)
    ++mid;

  std::vector<typename InputIt::value_type> res;
  mergesort(first, mid);
  mergesort(mid, end);
  my_merge(first, mid, mid, end, std::back_inserter(res));
  std::copy(res.begin(), res.end(), first);
}

int main()
{
  std::list<int> a { 1,2,8,4,2457,5,2,58,3,564,6,7 };
  mergesort(a.begin(), a.end());
  for(auto& i : a)
    std::cout << i << std::endl;
  return 0;
}

Questions:

Are there any obvious ways of improving my C++ implementation?
Are there any possible efficiency improvements? My annoyance is mostly that I walk any list twice for searching the midpoint. Is there a way of doing this without having to manually write a traversal of the list of two iterators, one moving one step at a time and the other two to get the midpoint in one sweep?
Do I have to give up generality in C++? I don't see how to do the linear "in-place merge" as with the C implementation unless we specialize. I'm not sure if it's possible to do an in-place merge of two STL lists either, since you don't have direct access to the next and previous pointers?

The loop

for(size_t i = 0; i < n/2; ++i)
    ++mid;

may and should be replaced with std::advance(mid, n/2)

To answer your question on better ways of finding the midpoint, I don't think it is worthy: it complicates the logic, while the amount of calculation remains the same.
A condition if(*first1 < *first2) breaks the stability of merge sort. This way, if two elements compared equal, the one from the second list would output first. Very standard mistake. To fix, either compare for <=, or change the order:

if (*first2 < *first1) {
    *out = *first2;
    ++first2;
} else {
    *out = *first1;
    ++first1;


The program that reverses bytes in two distinct ways (in C++) Will be grateful if anyone would reasonably comment on my code. I am interested in the style only. If there will be any comments which are not related to the style I am glad to read them as well.

   #include <iostream>

    unsigned reverse0(unsigned);

    union FourBytes
    {
        unsigned _value;
        struct Word
        {
            unsigned b1 : 8;
            unsigned b2 : 8;
            unsigned b3 : 8;
            unsigned b4 : 8;
        } _word;
    };
    unsigned reverse1(unsigned);

    int main(void)
    {
        int x(1), y(2);

        ++x = y;

        std::cout << reverse0(2447) << std::endl
            << reverse1(2447) << std::endl;
        return 0;
    }

    unsigned reverse0(unsigned p)
    {
        unsigned res(0);
        for (int i(0); i < sizeof(p)*8; ++i)
        {
            unsigned ithBit( (p & (1<<i)) >> i);
            int byteNumber(sizeof(p) - (i / 8) - 1);
            res |= (ithBit << (byteNumber*8+i%8));
        }
        return res;
    }

    unsigned reverse1(unsigned p)
    {
        FourBytes fb;
        fb._value = p;

        FourBytes res;
        res._word.b1 = fb._word.b4;
        res._word.b2 = fb._word.b3;
        res._word.b3 = fb._word.b2;
        res._word.b4 = fb._word.b1;

        return res._value;
    }

Style-wise, objectively improvable:

Avoid leaving dead code

This looks like junk you forgot to delete:

int x(1), y(2);

++x = y;

Comment non-trivial algorithms:

reverse0() is funky enough that I had to squint a bit and take a quick minute to run the code in my head to even have an idea of what it's trying to do. An explanation of what each line is "supposed" to be doing would have been helpful here.

Either name the struct type or the field name, not both

struct Word
{
    unsigned b1 : 8;
    unsigned b2 : 8;
    unsigned b3 : 8;
    unsigned b4 : 8;
} _word;

You don't need Word for anything, so you should just omit it entirely

Redundant explicit void argument list in main declaration:

int main(void) should just be int main()

Style-wise: opinions:

I don't like using unsigned

Since unsigned generally reads like a qualifier (I know it's not, it just looks like one), I personally much prefer being explicit and using unsigned int instead.

non-style stuff, but important:

don't use non-sized types with hard-coded sizeof() expectations.

Your FourBytes struct assumes that sizeof(unsigned) is 4, which is bad, you should be using std::uint32_t to properly enforce that.

Writing to an union field and reading from another one is illegal

This is surprising to many people as using unions to perform type punning is such a common technique, but it is, in fact, undefined behavior. You are only allowed to read from the last field you wrote to.

That means your reverse1() function is not valid C++ at all.

What's legal is reading and writing from a char (or unsigned char) array obtained from reinterpret_casting the pointer of a standard layout object. In fact, it's the only legal way to do direct memory manipulation of an existing object.

Edit: as requested, an implementation of reverse() using casting to char array:

unsigned int reverse(unsigned int v) {
   unsigned int result = 0;

   char const* src = reinterpret_cast<char const*>(&v);
   char* dst = reinterpret_cast<char*>(&result);

   for(int i = 0; i < sizeof(v); ++i) {
     dst[sizeof(v) - i - 1] = src[i];
   }

  return result;
}


A more efficient way of extracting a name from a string? Here is some code that reads Active Directory to get a person's manager and parses the result for the managers name. It omits the remaining information. I was just wondering if there might be some more efficient way of doing this instead of the code I am currently using.

var Manager = (string)UserDirectoryData.Properties["manager"].Value;
var StartIndex = Manager.IndexOf("=");
var EndIndex = Manager.IndexOf(",");
StartIndex++;
EndIndex--;
var Length = EndIndex - StartIndex;
Manager = Manager.Substring(StartIndex, Length);

The string returned in the first line of code is in this format:

  CN=Joseph Rod,OU=LaptopUser,OU=Users,DC=Company,DC=local

The goal is to just get the name and drop the rest. As the Name length will vary and my RegEX is rusty I could not come up with a working RegEX for this or any other way in C# to pull the name.

Edit:

Buy more efficient I mean faster run times and less resource utilization. This same code will be used in multiple spots in loops of up to 500 iterations (and it will continue to grow as the company does).

Here's a simple Regex to pull out the CN value:

CN=([^,]*),

You can use this C# code to get the value:

string CN = Regex.Replace(input, @"CN=([^,]*),", "$1");

Here are the important points:

I'm assuming you want everything between the "CN=" and the next comma.
The Regex replace allows a special string in the last parameter.  The "$1" will be replaced with all of the characters in between the parentheses.
You can test out the Regex here https://regex101.com/r/dT8lP8/1

In terms of computational efficiency, your solution is most likely more efficient than this Regex solution.  You might see an improvement if you replace the second through fifth lines of your solution with this:

var StartIndex = Manager.IndexOf("=") + 1;
var EndIndex = Manager.IndexOf(",", StartIndex) - 1;

I'm using this overload of the IndexOf method that tells the code at what index to start looking for the comma.  This starts the search at index 3 ("J") instead of index 0 ("C").  Again, this might run a little faster, but probably not measurably so.


kryptein cryptography package for Node.js I have written a cryptography NPM package called kruptein.  The code is also hosted on GitHub.

Having used multiple "best practices" with regards to key management, algorithm selection, key size, iv size etc. I could use someone with experience in this area to help validate the implementation.

I could use some help determining if I have flaws in any of the following:

Key sizes: _matrix(algo) - Are the appropriate key, iv & authentication tag sizes being identified and used correctly based on the algorithm selection? i.e. AES-128-CBC or AES-256-XTS

  _matrix(algo) {
    let obj = {
      at_size: 16,
      iv_size: 16,
      key_size: 32
    };

    if (algo.match(/ccm|ocb|gcm/i))
      obj.iv_size = 12;

    if (algo.match(/aes/) && algo.match(/ecb/))
      obj.iv_size = 0;

    if (algo.match(/aes/) && algo.match(/128/))
      obj.key_size = 16;

    if (algo.match(/aes/) && algo.match(/192/))
      obj.key_size = 24;

    if (algo.match(/aes/) && algo.match(/xts/))
      obj.key_size = 32;

    if (algo.match(/aes/) && algo.match(/xts/) && algo.match(/256/))
      obj.key_size = 64;

    return obj;
  }
}

Key derivation: _derive_key(secret) - When deriving keyed material from a provided secret are enough iterations used? Is there a problem with the salt generation used for the resulting keyed material? What about the resulting key? For both the IV and key I am concerned about possible weakening of the material when converting to a buffer and slicing to the appropriate sizes.

  _derive_key(secret) {
    let key, hash, salt, result, derived_key;

    try {
      hash = this.crypto.createHash(this.hashing);
      hash.update(secret);
      salt = hash.digest();
    } catch(err) {
      throw err;
    }

    salt = (Buffer.isBuffer(salt)) ?
      salt.slice(0, 16) : salt.substr(0, 16);

    key = this.crypto.pbkdf2Sync(secret, salt, 10000, 64, this.hashing);

    return Buffer.from(key.toString(this.encodeas)).slice(0, this.key_size);
  }

IV generation: _iv(iv_size) - When generating a new IV is there a problem with slicing the results. Most of the node.js crypto API functions use buffers and the sizes must match but I can't help but think it is weakening the IV.

  _iv(iv_size) {
    let iv = this.crypto.randomBytes(iv_size);

    return Buffer.from(iv.toString(this.encodeas),
      this.encodeas).slice(0, iv_size);
  }

think I should prefix this by saying I'm not a JS or Crypto nerd, but am interested in both!  to other people seeing this question, this code/module seems to mostly reimplement the CCM demo at the bottom of the Crypto docs:  https://nodejs.org/api/crypto.html#crypto_ccm_mode

anyway on with a few issues I noticed:

why isn't _iv just returning crypto.randomBytes(iv_size)?  encoding to a string and then getting bytes seems counterproductive at best, it just seems to throw away entropy
why use PBKDF2 over just hashing the secret?  it's not being saved anywhere, and by using the secret as the salt you've just opened it wide up to rainbow table exploitation again.  further, by doing your string encoding things you've just thrown away even more entropy
your use of regexes in _matrix looks suspect.  why not just match against a few common patterns (e.g. /^\w+-\w+-\w+/) and handle the groups explicitly.  allowing modes like ECB at all here looks like a mistake
catching exceptions just to immediately rethrow doesn't seem very useful
can you think of a better name for the member variable flag?  maybe something like is_aead_mode

update:

another big issue just occurred to me; that you're not including any cryptographic details in the structure.  this allows the suite used to be upgraded (e.g. NIST update their recommendations) while maintaining backwards compatibility.  i.e. you should be saying that you're hashing the secret with SHA512, which algorithm was used for encryption, what you've used as a HMAC (if you've used it, CCM already includes its own MAC so HMAC isn't needed).


Which one is more readable for mutually exclusive but mandatory command line parameters validation? I have a small validation code that needs to make sure that exactly one of two parameter is
set

I consider two options, the "Naive" that has some repetitions and might look "clumsy" but is very clear, and a second that is a bit more "DRY" but I'm not sure it's as readable / convincing in it's correctness.

Option one

if(param1==null && param2==null){
  throw new RuntimeException("Either param1 or param2 must be specified");
}

if(param1!=null && param2!=null){
  throw new RuntimeException("Only one of param1 or param2 must be specified");
}

if(param1!=null){
  //do something with param1
}
if(param2!=null){
  //do something with param2
}

Option two

if(param1!=null){
  if(param2!=null){
    //only runs if both are not null
    throw new RuntimeException("Only one of param1 or param2 must be specified");
  }
  //do something with param1
} else if(param2!=null){
  //do something with param2
} else{
  //only runs if both are null
  throw new RuntimeException("Either param1 or param2 must be specified");
}

Which one is more readable? which one would you keep? do you see any issue with Option 2?

Edit:

Based on feedback, I think there is a bit nicer option

Option three

boolean isParam1Valid = param1!=null;
boolean isParam2Valid = param2!=null;

if(isParam1Valid == isParam2Valid ){ //e.g. either both are null or both are not null
  throw new RuntimeException("param1 or param2 must be specified exactly once");
}

if(isParam1Valid){
  //do something with param1
}

if(isParam2Valid){
  //do something with param2
}

Option four shorter, but a bit less readable IMHO than option three (one needs to go up and read the code to understand why author is so sure that the else block means that param2 is valid

boolean isParam1Valid = param1!=null;
boolean isParam2Valid = param2!=null;

if(isParam1Valid == isParam2Valid ){ //e.g. either both are null or both are not null
  throw new RuntimeException("param1 or param2 must be specified exactly once");
}

if(isParam1Valid){
  //do something with param1
}else{ //must be true, as isParam1Valid is false, and they are not equal
  //do something with param2
}

I would put all validations first, and then extract to a method.  Better, trying using a parsing framework like JCommander that might be able to handle that for you, so that your code is just business logic


Python - Downloading map tiles I'm fairly new to programming and this is the 2nd thing I've done in python.

This code creates a /Tiles directory in the folder where the script is ran, then it makes an X (8) number of folders inside of /Tiles.

Inside of those 8 folders it makes a Y number of subfolders and downloads Z number of images per Y folder.

Y and Z are the same number but change depending on the X folder.

X folder 0 contains 2^0 Y folders and 2^0 Z images per Y folder.

Next X folder 2^1, etc until 8 folders or 2^7.

Any general or python specific advice for a newbie?

Also, any design advice?

I was thinking of having a loading bar of some sorts because it's ~20k images and was looking into this https://stackoverflow.com/questions/3173320/text-progress-bar-in-the-console

But in the end decided I just want to finish this up since I was done with the main functionality.

[..] means the code is on the same line but I made a new one for the purposes of reading here without scrolling.

Thanks.

import os
import urllib.request
import ctypes

print ("This script will download map tiles from [Redacted link]")
print ("The further it gets the more time it will take to fill the current folder.")
print ("Because the number of images increases exponentially.")
print ("You can check the progress by going into the latest folder.")
print ("When it is done the taskbar icon will flash and prompt you to close the window.")

# stores current directory
path = os.getcwd()
print ("The current working directory is {}.".format(path) + "\n")

# adds the names of the new directory to be created
path += "/Tiles/"

# defines the access rights
access_rights = 0o755

# creates the tiles directory
try:
    os.mkdir(path, access_rights)
except OSError:
    print ("Creation of the tiles directory {} failed.".format(path))
else:
    print ("All files will be downloaded to {}.".format(path))
    # for testing:
    # print ("Successfully created the tiles directory {}".format(path))

# number of x subfolders
x_folders = 8
# number of y subfolders and z images - different folders have a
[..] # different number of subfolders amd images
yz_array = [2**0, 2**1, 2**2, 2**3, 2**4, 2**5, 2**6, 2**7]

# main loop
# creates the x directories
for x in range(x_folders):
    try:
        os.mkdir("{}/{}".format(path,x), access_rights)
    except OSError:
        print ("Failed to create the X directory - {}.".format(x))
        break
    except:
        print("Something went wrong creating X folders.")
        break
    else:
        print("Downloading images to {}{}.".format(path,x))

        # creates the y directories
        for y in range(yz_array[x]):
            try:
                os.mkdir("{}/{}/{}".format(path,x,y), access_rights)
            except OSError:
                print ("Failed to create the Y directory - {}.".format(y))
                break
            except:
                print("Something went wrong creating Y folders.")
                break
            else:
                pass

            # downloads z images
            for z in range(yz_array[x]):
                try:
                    urllib.request.urlretrieve("[Redacted link]
[..]/{}/{}/{}.png" \
                    .format(x,y,z), "{}/{}/{}/{}.png".format(path,x,y,z))
                except:
                   print("Something went wrong downloading Z images.")
                   break
                else:
                    pass

    print("Successfully filled folder {}{}.".format(path,x) + "\n")

print("Finished running.")
ctypes.windll.user32.FlashWindow(ctypes.windll.kernel32.
[..]GetConsoleWindow(),True) # flashes the taskbar icon
print("Press Enter to close")
input("\n")

Cool python exercise! And a nice job.
I'm going to assume you are using python3. There are some points of improvement:

Multiline strings: In python you can create a string with more than one line using """ so you an put all of the printing in the beginning of your program in one print statement.
Pathlib: There is a library called pathlib which allows you to create multi platform path objects. Try to use it instead of concatenating strings yourself.
Constants in capitals: Is access_rights a constant? Write it in capital letters if so (It's a normal naming convention).
String formatting: You can use fstrings instead of format, it looks much nicer and more readable (to me). So "Downloading images to {}{}.".format(path,x) can now be f"Downloading images to {path}{x}.".
List comprehension: you can create the yz_array using list comprehension, it is a bit complicated but it will look like: [2 ** x for x in range(8)] - will get the same result!
Meaningful variable names: instead of x even use x_index.
Meaningful exceptions: Using Exception as e you can print the system error, giving the user more information.
Logical error: Notice that even if there was an error in the file creation process you will still print "Successfully filled folder" which is not true. Add a boolean variable to prevent that.
Multi Platform compatibility: You are probably using windows, but on other os your code will crush, by adding another try - catch you can prevent that. You can also try and find the os and create a matching case for windows.

Maybe:

Functions: You can use functions to avoid the indentations.
Spaces: According to PEP8 you should add a space after a , and you should not add space after a print statement.
Use type annotation: (Last but this one is my personal favourite) When your code gets complicated and you start to use function - Do not forget to read about type annotations.

Overall - greate code, nicely written.
Good luck and Have fun!


Condensing Navigation jQuery I have some jquery that pretty much just runs a small drop-line navigation. Each tab size differs, so i used the same jquery line 5 times to make it look and function the way i wanted it to. I'm sure there is a way to condense it but im not quite a pro. Can anybody help me out a bit? The jquery is below, and here is the site for reference: http://solocreativegroup.com/ed_spa/index.php. Thanks!

here's the code that runs my drop-line nav.

$(document).ready(function() {

$("ul#topnav li.home").hover(function() { //Hover over event on list item
    $(this).css({ 'background' : 'url(images/tab_hobt.png) no-repeat top left'}); //Add background color + image on hovered list item
    $(this).find("span").show(); //Show the subnav
}, function() { //on hover out...
    $(this).css({ 'background' : 'none'}); //Ditch the background
    $(this).find("span").hide(); //Hide the subnav
});

$("ul#topnav li.about").hover(function() {
    $(this).css({ 'background' : 'url(images/tab_hobt.png) no-repeat top left'});
    $(this).find("span").show();
    $('.services a').css({'color':'#ffffff'});
}, function() {
    $(this).css({ 'background' : 'none'});
    $(this).find("span").hide();
    $('.services a').css({'color':'#921d49'});
});

$("ul#topnav li.services").hover(function() {
    $(this).css({ 'background' : 'url(images/tab_services.png) no-repeat top left'});
    $(this).find("span").show();
    $('.services a').css({'color':'#ffffff'});
}, function() {
    $(this).css({ 'background' : 'none'});
    $(this).find("span").hide();
    $('.services a').css({'color':'#921d49'});
});

$("ul#topnav li.testimonial").hover(function() {
    $(this).css({ 'background' : 'url(images/tab_testimonials.png) no-repeat top left'});
    $(this).find("span").show();
}, function() {
    $(this).css({ 'background' : 'none'});
    $(this).find("span").hide();
});

$("ul#topnav li.contact").hover(function() {
    $(this).css({ 'background' : 'url(images/tab_contact.png) no-repeat top left'});
    $(this).find("span").show();
}, function() {
    $(this).css({ 'background' : 'none'});
    $(this).find("span").hide();
});

HTML:

HOME

ABOUT

                PRODUCTS

            SERVICES

                FACIALS
                AGE MANAGEMENT
                MICRODERMABRASION
                SKIN RESURFACING
                ADD IT ON
                WAXING

TESTIMONIALS

CONTACT

You're jQuery can be written in one simple function as a catch all for the entire navigation. As you stated you think it could be condensed, you'd be right. As a rule of thumb if you see yourself writing the same thing over and over you should be able to cut it all down into one function.

Starting with one of your methods I'll step through it to show how you can condense this and make it more efficient.

$("ul#topnav li.home").hover(function() { //Hover over event on list item
    $(this).css({ 'background' : 'url(images/tab_hobt.png) no-repeat top left'}); //Add      background color + image on hovered list item
    $(this).find("span").show(); //Show the subnav
}, function() { //on hover out...
    $(this).css({ 'background' : 'none'}); //Ditch the background
    $(this).find("span").hide(); //Hide the subnav
});

First things first, you're creating no less than 5 jQuery objects for each time someone hovers and moves out of a menu item.

Two things on this :

Don't use jQuery where you don't need it
Cache jQuery objects for re-use
Make your code scalable

Don't use jQuery to...

Modify CSS unless it is absolutely required (ie CSS alone can't achieve what you want). In this case CSS can handle you're requirements easily.

If you find you need to change a lot of CSS and would like this CSS to change due to a jQuery or javascript event. use .addClass() and .removeClass(). Applying the styles in CSS rather than in jQuery. This is far more efficient in terms of code and your eyes. :)

Cache your jQuery objects

$(this).css(...);
$(this).find("span");

would be better served with one cached object

var self = $(this);

self.css(...);
self.find("span");

Make your code scalable

By this I mean using techniques that allow you to do some relatively advanced things without the need for re-writes / re-factors in the future. One of these is to bind events to a lower level element with .on rather than .hover or .click. If in the future you modify the menu for some reason with jQuery/JS and DOM elements are removed / re-added, event bindings will be lost. Using .on avoids this problem.

Refactoring your code

I've deliberately not included any CSS changes in this code as you can and should include these in your CSS rather than dynamically with jQuery / JS. I have however added and removed a class, this should contain all the style changes required for the mouseenter and mouseleave events.

$("#topnav").on({
    mouseenter : function(event)
    {
        var self = $(this),
            menu = self.find("span");

        // Check if a menu is present and show it
        if (menu.length > 0)
        {
            menu.show();

            // Add an is-active class to the menu link
            menu.siblings("a").addClass('is-active');
        }
    },
    mouseleave : function(event)
    {
        var self = $(this),
            menu = self.find("span");

        // Check if a menu is present and hide it
        if (menu.length > 0)
        {
            menu.hide();

            // Remove is-active class on the menu link
            menu.siblings("a").removeClass('is-active');
        }
    }
}, "li");

I hope this helps you!


TypeScript implicit 'any' type with computed property key (irrelevant code removed for demo purposes)

I am new to using TypeScript and I have this React component which has an object literal inside the component, which I'm then accessing using a computed property key. Everything works as expected but I was getting the following TS error:

Element implicitly has an 'any' type because expression of type 'string' can't be used to index type '{ keyword: string; hex: string; rgb: string; }'.
No index signature with a parameter of type 'string' was found on type '{ keyword: string; hex: string; rgb: string; }'.ts(7053)

I managed to get rid of the error by adding the : any to the formats object, though I think this is not the best solution?

interface Props {
  keyword: string;
  hex: string;
  rgb: string;
  copyFormat: string;
}

export default ({ keyword, hex, rgb, copyFormat }: Props) => {
  const formats: any = {
    keyword,
    hex: hex.toUpperCase(),
    rgb: `rgb(${rgb})`,
  };

  return <p>{formats[copyFormat]}</p>;
};

You very rarely need to use any in TypeScript because its type-system is so expressive.

The problem is that copyFormat is specified to be any string.

What would happen if you passed "blahblahblah" as copyFormat? Your types currently allow this!. Then, your code would attempt to do formats["blahblahblah"], but .blahblahblah is not a member of  the formats object.

You need a more precise type for copyFormat. You intended the caller to only pass one of "keyword", "hex", or "rgb". You can make a type that is just those three values like so:

interface Props {
  keyword: string;
  hex: string;
  rgb: string;
  copyFormat: "keyword" | "hex" | "rgb";
}

It looks like Props really holds two things; it holds the actual data, and then a separate argument controlling what you read from it. It might make sense to split it into two arguments:

export default ({ keyword, hex, rgb }: Props, copyFormat: "keyword" | "hex" | "rgb") => {
  const formats = {
    keyword,
    hex: hex.toUpperCase(),
    rgb: `rgb(${rgb})`,
  };

  return
{formats[copyFormat]}
;
}

If you don't like typing out all of the possible key options, you could instead use the keyof type operator to say that copyFormat is any valid key in the Props type. TypeScript will see that all of the properties are strings and correctly infer the resulting type of formats[copyFormat] is thus a string.

interface Props {
  keyword: string;
  hex: string;
  rgb: string;
}

export default ({ keyword, hex, rgb }: Props, copyFormat: keyof Props) => {
  const formats = {
    keyword,
    hex: hex.toUpperCase(),
    rgb: `rgb(${rgb})`,
  };

  return
{formats[copyFormat]}
;
};
```


Find all instances of element in array I've created a function to return all the indices where an element is to be found in an array. If it occurs twice or more, all the respective indices will be returned, for now it returns them in natural counting (1,2,3...) and not how the computer sees them (0,1,2,...).

#include <iostream>
#include <vector>
using namespace std;

void searchList(int theArray[], int sizeOfTheArray, int findFor);

void searchList(int theArray[], int sizeOfTheArray, int findFor){
  vector<int> foundIndices;

  int j = 0;

  for (int i = 0; i < sizeOfTheArray; i++)
  {
    if (theArray[i] == findFor){
      foundIndices.push_back(i);
      j++;
    }
  }

  if (foundIndices.size()!=0){
    cout << "Found in index: ";
    for (int i = 0; i < foundIndices.size(); i++){
      cout << foundIndices[i]+1 << " ";
    }
  }
  else
    cout << "Not found in array";
}

int main(){
  int test[8] = {87, 75, 98, 100, 100, 234, 265, 9};
  searchList(test, 8, 99);
  return 0;
}

Separation of Concerns

The function should have only one concern--finding the indices. Printing out results is a separate concern that shouldn't be combined with finding the indices. Combining the two makes it difficult or impossible to use in a wide variety of situations.

Vacuous Declaration

Having the declaration followed immediately by the definition renders the declaration just extra visual noise.

Pointless Work

In your function, you initialize and increment j, but you never seem to make any use of it.

Inconsistent Types

The type normally used for indices into arrays is size_t, but you're returning a vector of int instead.

Intermixing Arrays and Vectors

Although it's not exactly an error, it is a bit strange to have an array as the input type, and a vector as the result type. Typically you'd want to pick one or the other, and use it consistently. The other obvious possibility would be to have the function itself work with iterators, so it doesn't have to specify the type of the collection itself at all (and can be written generically, so it can work with essentially any collection).

const Correctness

Since this function shouldn't modify the input array, it's better to pass that as a pointer to const (note: although your parameter definition uses array notation, what's passed is actually a pointer). If you use a vector as input, you'd probably want to pass it as a reference to const.


Dining Philosopher correctness How should I improve my solution for Dining Philosopher's problem?

#include "stdafx.h"
#include "iostream"
#include "vector"
#include "algorithm"
#include "mutex"
#include "chrono"
using namespace std;

class Dining {
public:
    static int starved_threads;
private:
    std::recursive_mutex arbit;
    std::vector<int> chopsticks;
    int count;
public:
    Dining()
    {
        vector<int> v = { 1,2,3,4,5 };
        chopsticks = v;
        count = 0;
    }
private:
    int acquire()
    {
        arbit.lock();
        int x = chopsticks[count%chopsticks.size()];
        chopsticks[count%chopsticks.size()] = 0;
        count++;
        arbit.unlock();
        return x;

    }
    void release(int i)
    {
        arbit.lock();
        chopsticks[i - 1] = i;
        arbit.unlock();
    }

public:
    void eatThink() {
        while (true) {
            int left = 0, right = 0;
            left = acquire();
            if (left != 0)
                right = acquire();
            else {
                if (left != 0)
                    release(left);
            }
            arbit.lock();
            if (left && right)
            {
                cout << "Thread " << std::this_thread::get_id() << "Eating" << endl;
                std::this_thread::sleep_for(std::chrono::milliseconds(10));
                cout << "Thread " << std::this_thread::get_id() << "Releasing" << endl;
            }
            arbit.unlock();

            if (left > 0)
            {
                release(left);
            }

            if (right > 0)
            {
                release(right);
                arbit.lock();
                starved_threads++;
                cout << starved_threads;
                arbit.unlock();
                return;
            }
        }
    }
};

int Dining::starved_threads = 0;

int main()
{

    Dining* philosophers = new Dining();
    int size = 20;
    std::vector<std::thread> threads(size);
    for (int i = 0; i < 5; i++) {
        for (int i = 0; i < size; i++) {
            threads[i] = std::thread(&Dining::eatThink, philosophers);
        }
        for (int i = 0; i < threads.size(); i++) {
            threads[i].join();
        }
    }
    return 0;
}

Dinning Philosophers

How should I improve my solution for Dining Philosopher's problem?

Well first implementing the Dining Philosophers problem would be good. This is an interesting problem you implemented but its not the classic Dinning Philosophers
The issue here is that in your implementation each philosopher tries to grab any two forks. In the classic implementation each philosopher is assigned two specific forks (though each is shared with another philosopher).
The other issue is that you have a single lock for the whole table. In reality you should have a lock on each fork. Just because philosopher one is grabbing fork one does not stop philosopher three grabbing fork three.
Also if a philosopher fails to get either fork, then it must give up all forks and think for a random period.
Code Review
Prefer initializer list. Also you don't need a temporary v in the constructor.
Dining()
{
    vector<int> v = { 1,2,3,4,5 };
    chopsticks = v;
    count = 0;
}

Dining()
    : chopsticks{ 1, 2, 3, 4, 5}
    , count(0)
{}

Use RIAA.
If you have code that looks like
InitSomething

// Stuff

UnitSomething

Then you should have an object that handles the InitSomething in the constructor and UnitSomething in the destructor.
In this:
    arbit.lock();

    // STUFF

    arbit.unlock();

Should look like this:
    std::lock_guard<std::recursive_mutex>   lock(arbit);
    // STUFF

Don't use dynamic allocation when automatic variables will work
Dining* philosophers = new Dining();

This should be:
Dining   philosophers;


Determining whether one vector is a prefix of the other I am learning from C++ Primer 5th edition. This is exercise 5.17:

  Given two vectors of ints, write a program to determine whether one vector is a prefix of the other. For vectors of unequal length, compare the number of elements of the smaller vector. For example, given the vectors containing 0, 1, 1, and 2 and 0, 1, 1, 2, 3, 5, 8, respectively your program should return true.

#include <iostream>
#include <string>
#include <vector>

using std::cin;
using std::cout;
using std::endl;
using std::string;
using std::vector;
using std::end;
using std::begin;

int main()
{
    vector<int> i{0,1,1,2}, n{0,1,1,2,3,5,8}, smlr;
    char sml, bgr;
    long j = (i.end()-i.begin()) ,k = (n.end()-n.begin());
    if ( j > k)
    {
        sml = 'n', bgr = 'i', smlr = n;
    }

    else if (j < k)
    {
        sml = 'i', bgr = 'n', smlr = i;
    }
    else if ( j == k)
    {
        cout << "both are of equal length";

        for (vector<int>::size_type a = 0; a < i.size() ; ++a ) {
            if (i[a] != n[a]) {
                cout << "both vectors are unequal and are not prefixes of the other";
                return -1;
            }
        }
    }

    for ( int l = 0; l < smlr.size() && i[l] == n[l]; ++l )
    {
            if(l == (smlr.size()-1))
            {
            cout << sml << " is a prefix of " << bgr;
            return 0;
            }
    }
    cout << sml << " is not a prefix of " << bgr;
    return 0;
}

Is my code efficient?
Am I using too many variables?
Is it better to write more code and use less variables or use less variables and less code?

I am a beginner, so please be harsh.

Ok first, we want to determine if some condition is true or not based on some input. That calls for a boolean function! In particular, our inputs are two vectors. :

bool isPrefixOf(const std::vector<int>& smaller, const std::vector<int>& larger)
{
    ...
}

I named the first variable smaller, but it might not actually be smaller! Let's make sure that it is:

    if (smaller.size() > larger.size())
    {
        std::swap(smaller, larger); // this very efficiently swaps our vectors
                                    // in constant time
    }

Now once we got that, the algorithm in your j == k is exactly correct, but pretty hard to read. The convention is to use variables like i, j, k solely as indices in loops, not as important variables. So I'll rewrite it like this:

    for (int i = 0; i < smaller.size(); ++i)
    {
        if (smaller[i] != larger[i])
        {
            return false; // clearly not a prefix
        }
    }

    return true; // we match up, so we must be done!
}

So cool, we got our function that checks if a smaller vector is a prefix of a larger vector, so now our main just has to determine which way we call it:

int main()
{
    vector<int> vec1{0, 1, 1, 2};
    vector<int> vec2{0, 1, 1, 2, 3, 5, 8};

    cout << "vec1 is a prefix of vec2?: " << isPrefixOf(vec1, vec2) << endl;
}

Which should print true. So in general, your variable names don't line up with convention, and you coded the same logic twice - don't repeat yourself! Take advantage of functions to encode conceptually distinct functionality.


C# Ping - Checking availability of hosts Simple Console Application for checking if one of the hosts is available. The implementation should be for around 5 hosts to check with different response times and the method should return fast when one Host can be pinged. Is the asynchronous implementation O.K.?

using System;
using System.Net.NetworkInformation;
using System.Threading;
using System.Threading.Tasks;

namespace PingMe
{
    class Program
    {
        private static volatile bool _result;

        private readonly CancellationTokenSource _source;

        private readonly ParallelOptions _parallelOptions;
        private readonly string[] _hostsToPing = {"google.com", "reddit.com", "netflix.com"};

        private Program()
        {
            _source = new CancellationTokenSource();
            var token = _source.Token;
            _parallelOptions = new ParallelOptions
            {
                CancellationToken = token
            };
        }

        private bool IsOneOfTheHostsAvailable()
        {
            try
            {
                Parallel.ForEach(_hostsToPing, _parallelOptions, (host) =>
                {
                    try
                    {
                        var ping = new Ping();
                        var reply = ping.Send(host);
                        _parallelOptions.CancellationToken.ThrowIfCancellationRequested();
                        if (reply?.Status == IPStatus.Success)
                        {
                            _result = true;
                            Console.WriteLine($"Pinged {host}");
                            if (!_source.IsCancellationRequested)
                                _source.Cancel();
                        }
                    }
                    catch (Exception)
                    {
                        // we are only interested if one of the pings is successful, we don't care about the reason
                    }
                });
            }
            catch (OperationCanceledException)
            {
                // we are done
            }

            return _result;
        }

        private static void Main()
        {
            var program = new Program();
            var oneOfTheHostsIsAvailable = program.IsOneOfTheHostsAvailable();
            Console.WriteLine($"One of the hosts is available: {oneOfTheHostsIsAvailable}");
        }
    }
}

It doesn't make sense to have the CancellationTokenSource private like you have now. Who is going to be able to cancel the operation? The user of your class can't access the source or provide their own token. The class doesn't offer any API to cancel.

Instead, you should alter the private bool IsOneOfTheHostsAvailable() method to accept a CancellationToken as input. You can then let the user care about creating a source and possibly cancelling the operation. Or, if you don't feel like offering the option to the user, you should just do away with the cancellation token as a whole. Right now it isn't doing anything.


Hexadecimal to RGB conversion I am trying to convert hex to rgb, and rgb to hex. My current code doesn't seem optimal, and there is an issue with 0's in the hexadecimal. What do you think about the overall quality of this code? Can you think of a more efficient way of converting?

  $scope.$watch('hex', function() {
    var rgb = parseInt($scope.hex, 16);
    $scope.r = (rgb >> 16) & 0xFF;
    $scope.g = (rgb >> 8) & 0xFF;
    $scope.b = rgb & 0xFF;
    $scope.rgb = 'rgb(' + $scope.r + ',' + $scope.g + ',' + $scope.b + ');';
  });

  $scope.$watch('r+g+b', function() {
    $scope.rgb = 'rgb(' + $scope.r + ',' + $scope.g + ',' + $scope.b + ');';
    $scope.hex = parseInt($scope.r << 16 | $scope.g << 8 | $scope.b).toString(16);
  });

Here is a sample plunker:

A few notes:

I am confused as to why you need AngularJS for this.
I prefer to use the bitwise & with 255 instead of 0xFF in this case.
Your function can't handle non-hex characters such as #.  I would remove them before parsing.
You can simplify your creation of the RGB string by using the join() method.

Final function:

function hexToRgb(hex) {
    hex = hex.replace(/[^0-9A-F]/gi, '');
    var bigint = parseInt(hex, 16);
    var r = (bigint >> 16) & 255;
    var g = (bigint >> 8) & 255;
    var b = bigint & 255;

    return [r, g, b].join();
}


Is this implementation of factory design pattern in C#? Here are my class library code which create credit card.
public abstract class CreditCard
{
    public CardType CardType { get; protected set; }
    public decimal CreditLimit { get; protected set; }
    public decimal AnnualCharge { get; protected set; }
}

public class GoldCreditCard : CreditCard
{
    private GoldCreditCard(decimal creditLimit, decimal annualCharge)
    {
        CardType = CardType.Gold;
        CreditLimit = creditLimit;
        AnnualCharge = annualCharge;
    }

    /// <summary>
    /// Factory pattern
    /// </summary>
    /// <returns></returns>
    public static CreditCard Create()
    {
        return new GoldCreditCard(creditLimit: 15000, annualCharge: 100);
    }
}

public class TitaniumCreditCard : CreditCard
{
    private TitaniumCreditCard(decimal creditLimit, decimal annualCharge)
    {
        CardType = CardType.Titanium;
        CreditLimit = creditLimit;
        AnnualCharge = annualCharge;
    }

    /// <summary>
    /// Factory pattern
    /// </summary>
    /// <returns></returns>
    public static CreditCard Create()
    {
        return new TitaniumCreditCard(creditLimit: 30000, annualCharge: 250);
    }
}

and the ui will be:
    static void Main(string[] args)
    {
        CreditCard card;

        System.Console.WriteLine("Select your card type");
        System.Console.WriteLine("1. Gold Credit Card");
        System.Console.WriteLine("2. Titanium Credit Card");
        var option = System.Console.ReadLine();
        switch (option)
        {

            case "1":
                card = GoldCreditCard.Create();
                PrintCard(card);
                break;
            case "2":
                card = TitaniumCreditCard.Create();
                PrintCard(card);
                break;
            default:
                break;
        }

    }

    static void PrintCard(CreditCard card)
    {
        System.Console.WriteLine($"Your credit card has been successfully created.");
        System.Console.WriteLine($"The credit card type is {card.CardType}.");
        System.Console.WriteLine($"The credit card limit is {card.CreditLimit:C}.");
        System.Console.WriteLine($"The credit card annual fee is {card.AnnualCharge:C}.");
    }

Is this implementation of factory design pattern in C#?

In your implementation you have smashed together two separate roles: Product and Creator.
Your implementation is equivalent with the following:
public class GoldCreditCard : CreditCard
{
    private GoldCreditCard(decimal creditLimit, decimal annualCharge)
    {
        CardType = CardType.Gold;
        CreditLimit = creditLimit;
        AnnualCharge = annualCharge;
    }

    public GoldCreditCard()
    {
        CardType = CardType.Gold;
        CreditLimit = 15000;
        AnnualCharge = 100;
    }
}

Or almost with this one (this provides more flexibility):
public class GoldCreditCard : CreditCard
{
    public GoldCreditCard(decimal creditLimit = 15000, decimal annualCharge = 100)
    {
        CardType = CardType.Gold;
        CreditLimit = creditLimit;
        AnnualCharge = annualCharge;
    }
}

The whole point of the pattern is that only the Creator (Factory) knows how to setup a concrete product. In your case your class knows how to initialize itself.


Simple alphabet scrambler --- usage of vectors I am currently learning C++, using this book: C++ Primer, 5th Edition. I made this program to help me understand the usage of vectors in C++. I have prior programming experience (mainly Python), and this just seems way too messy to be the right way of doing things. If I'm doing this incorrectly, or inefficiently, then please guide me in the right direction.

The purpose of this program is to take user input, and convert it into random letters (yes, I know that there is a lack of support for #'s and symbols, but that does not matter), somewhat simulating a hashing system.

This isn't homework, and it's not a dire issue. The program runs, but I want to make sure (since I'm new to C++), that I'm doing things in a somewhat correct way.

#include <iostream>
#include <string>
#include <vector>
int main(){
    srand(time(0));
    std::cout << "Welcome, to the scrambler: " << std::endl;
    std::cout << "Enter the string(all lowercase letters) you want to be scrambled: " << std::endl;
    std::string user_password;
    std::cin >> user_password;

    std::cout << "Select the type of encryption you want: " << std::endl;
    std::cout << "1. Standard alphabet scrambler " << std::endl;
              //Add support for other types...
    int choice = 0;
    std::cin >> choice;

    if (choice == 1) {

        std::vector<char> vuser_pass;

        for ( decltype(user_password.size()) i = 0; i <= user_password.size(); ++i ) {
            char letter_hold = user_password[i];
            vuser_pass.push_back(letter_hold);
        }
        //for ( decltype(vuser_pass.size()) i = 0; i <= vuser_pass.size(); ++i) {
            //std::cout << vuser_pass.at(i) << std::endl;
        //}

        std::string alpha = "abcdefghijklmnopqrstuvwxyz";
        std::vector<char> valpha;

        for ( decltype(alpha.size()) cnt = 0; cnt <= alpha.size(); ++cnt) {

            valpha.push_back(alpha[cnt]);
        }

        //for ( decltype(valpha.size()) cnt = 0; cnt <= valpha.size(); ++cnt) {
            //std::cout << valpha[cnt] << std::endl;

        //}

        auto limit = vuser_pass.size();
        limit -= 1;

        for ( decltype(vuser_pass.size()) sindex = 0; sindex < limit; ++sindex ) {
            decltype(valpha.size()) vindex = rand() % 25;
            vuser_pass[sindex] = valpha.at(vindex);
        }

        for ( decltype(vuser_pass.size()) i = 0; i < vuser_pass.size(); ++i) {
            std::cout << vuser_pass.at(i);
        }

    }
return 0;
}

Before discussing whether the algorithm is good or not, there are many small things that you could improve to make the code more readable:

Your program won't compile with some compilers if you don't explicitly include <cstdlib> for rand and srand and <ctime> for time.
It is good practice to always fully qualify with std:: the names of the standard library components you use. Therefore, you should explicitly write std::rand, std::srand and std::time.
Don't keep commented out comments. They will hardly help you and will probaby just be in the way. If you need to look again at old pieces of code, revision control software are there for you.
This loop:

std::vector<char> vuser_pass;
for ( decltype(user_password.size()) i = 0; i <= user_password.size(); ++i ) {
    char letter_hold = user_password[i];
    vuser_pass.push_back(letter_hold);
}

If I am not mistaken, you are simply asiigning every element of the string user_password to the vector vuser_password. In this case, please use the std::vector's constructor which takes two iterators:

std::vector<char> vuser_pass(std::begin(user_password), std::end(user_password));

This do what you need and you will avoid having to manually write a for loop. Note that it is also valid for the alpha/valpha conversion.
That said, do you need std::vectors at all? You can as well iterator directly on std::string since it implements the containers methods.
This loop:

for ( decltype(vuser_pass.size()) i = 0; i < vuser_pass.size(); ++i) {
    std::cout << vuser_pass.at(i);
}

If you use get rid of the vectors and simply use strings, you can directly output the resulting string to std::cout:

std::cout << user_password;

You don't need to return 0; at the end of main. If nothing is returned, the compiler wil automagically add a return 0; for you at the end. Not wirting it by yourself amy help to document the fact that you won't ever be returning anything else than 0.
You could a range-based for loop to simplify the latest remaining loop:

for (char& c: user_password) {
    c = alpha[rand() % 25];
}

Also, std::rand and std::srand are kind of advised against in modern C++ since there are safer alternatives in the new <random> header. However, the alternative is not that easy to use yet.

std::random_device rd;
std::mt19937 gen(rd());
std::uniform_int_distribution<> dis(0, 25); // will produce ints in [0, 25]

Now, you can create random integers between 0 and 25 by calling dis(gen).
Since alpha is not meant to be modified, it would be better to make it const.

So, let's take all of these modifications into account and write your code again:

#include <algorithm>
#include <iostream>
#include <random>
#include <string>

int main(){

    // random number generator in [0, 25]
    std::random_device rd;
    std::mt19937 gen(rd());
    std::uniform_int_distribution<> dis(0, 25);

    std::cout << "Welcome, to the scrambler: " << std::endl;
    std::cout << "Enter the string (all lowercase letters) you want to be scrambled: " << std::endl;
    std::string user_password;
    std::cin >> user_password;

    std::cout << "Select the type of encryption you want: " << std::endl;
    std::cout << "1. Standard alphabet scrambler " << std::endl;
              //Add support for other types...
    int choice = 0;
    std::cin >> choice;

    if (choice == 1) {

        const std::string alpha = "abcdefghijklmnopqrstuvwxyz";

        for (char& c: user_password) {
            c = alpha[dis(gen)];
        }

        std::cout << user_password;
    }
}

Voilà! Your program has been converted to somewhat clean modern C++. Now, all you have to do is to provide more scrambling algorithms and to provide a mechanism to tell the user that they have chosen an invalid option and that they should chose again.


Javascript inheritance I have a simple class inheritance in JS:

var Class = function(){};

Class.extend = function(){

    var Extended = function(constructor){

        function extend_obj(destination, source) {
            for(var property in source){
                if(typeof source[property] === "object" && source[property] !== null && destination[property]){
                    extend_obj(destination[property], source[property]);
                }else{
                    destination[property] = source[property];
                }
            }
        };

        extend_obj(Extended.prototype, constructor);

        this.constructor.init();
    };

    Extended.init = function(){};

    for(var key in this.prototype){
        Extended.prototype[key] = this.prototype[key];
    }

    Extended.prototype.constructor = Extended;

    for (var key in this) {
        Extended[key] = this[key];
    };

    return Extended;
};

Which allow me to do so:

var SubClass = Class.extend();

SubClass.static_property = "a static property";

SubClass.static_method = function(){
    return "a static method";
};

SubClass.prototype.a_new_property = "something new"; //instance method

var instance = new SubClass({
    name: "custom name"
});

SubClass.init = function(){
    //custom constructor
};

this.constructor.static_method_or_property //call instance method

I need:

multiple inheritance
the possibility to use module pattern

There's a cleaner/better/faster way to do that?

First of all, it seems like you're reinventing the wheel here. You didn't provide any context for what this is for so I'll try to cover all bases.

If you are using this as part of a bigger project, I'd recommend abandoning this approach and picking up something like Prototype.js or maybe Ember.js if you're ambitious. Alternatively, take a look at how CoffeeScript does it. CoffeeScript's underlying JS is pretty messy but it's significantly simpler than yours AND supports inheritance.

If this is just an educational exercise, I can point out a few problems with your code that I see.

extend_obj(Extended.prototype, constructor);

This seems to be copying the instance's properties onto the class prototype. This doesn't make much sense as the instance's properties should be separate.

var instance = new SubClass({ name: "custom name" })

This actually instantiates an Extended (aka SubClass) object with no instance properties and modifies the Extended prototype to have a name property of "custom name". This is very strange behavior.

In general, when trying to emulate Classes in JS, the prototype is where instance methods go or, possibly, default values for properties. However, an instance (created with new) should have no effect on it.


Singleton rewrite based on old one I inhereted an implementation of a singleton template.  Althoug I read that singletons are bad, my program uses singletons throughout and I can not get rid of them at the moment.

This is the implementation I started with:

template <typename T>
class Singleton
{
public:
    static T* ms_singleton;

    Singleton()
    {
        assert(!ms_singleton);
        long offset = (long)(T*) 1 - (long)(Singleton <T>*)(T*) 1;
        ms_singleton = (T*)((long) this + offset);
    }

    virtual ~Singleton()
    {
        assert(ms_singleton);
        ms_singleton = 0;
    }

    static T &instance()
    {
        assert(ms_singleton);
        return (*ms_singleton);
    }

    static T &Instance()
    {
        assert(ms_singleton);
        return (*ms_singleton);
    }

    static T* instance_ptr()
    {   return (ms_singleton); }
};

template <typename T>
T* Singleton <T>::ms_singleton = NULL;

And the new implementation.

#pragma once

template<typename T>
class Singleton
{
public:
    static T &Instance()
    {
        static T* myInstance;
        return (*myInstance);
    }

    Singleton(Singleton const &) = delete;
    Singleton(Singleton &&) = delete;
    Singleton &operator=(Singleton const &) = delete;
    Singleton &operator=(Singleton &&) = delete;

protected:
    Singleton() = default;
    virtual ~Singleton() = default;
};

The first code doesn't create the class and only sets a pointer to an existing instance; the new implementation does the same.

Do you think the second implementation is correct? Is there something that should be optimized or changed? There may be problems if I use it like that? Should I also handle the  creation of classes?

This new implementation should behave the same as first, because I have a big project and I just can't get rid of this singleton.

How good is the new singleton?

Toby Speight's answer is correct for a normal singleton, but the class that you're replacing doesn't look like a normal singleton.

The original class appears to allow the user to explicitly define the object lifetime. The static variable is actually just for ease of access to that object:

The new code changes the point of construction to the first call to Instance().
The point of destruction is changed from reverse-order of construction inside the owning scope, to reverse-order of construction when C++ cleans up static variables.

This may or may not matter to your use case, but note that these two factors combined can completely alter the order of destruction of your "singleton" objects.

If you want to keep managing the object lifetimes explicitly, it might be better to use something like the Service Locator pattern instead (which is closer to what the original code does).

A related example from my own code (the shared / weak ptrs could be replaced with raw pointers or some sort of handle):

template<class T>
class Service
{
public:

    static void Provide(std::weak_ptr<T> service)
    {
        if (Service::service.lock())
            throw std::runtime_error("Service is already being provided!");

        Service::service = service;
    }

    static void Stop()
    {
        Service::service = std::weak_ptr<T>();
    }

    static T& Get()
    {
        auto strongService = service.lock();

        if (!strongService)
            throw std::runtime_error("Attempting to use a null service!");

        return *strongService;
    }

private:

    Service() = delete;

    static std::weak_ptr<T> service;
};

template<class T>
std::weak_ptr<T> Service<T>::service;