Calculating multiple car prices from a file My application needs to read a file, which includes a list of cars from different factories, and calculate the prices (by this I mean mean add up their prices).

I suppose my solution is efficient.  Is there any better approach to it?

Company Price  other details
Benz   10000   x ...
BMW    3000    y ...
Benz   1000    z ...
Toyota 500     w ...

In this case the result should be

Benz 11000
BMW  3000
Toyota 500

Solution

public static final void main(String[] args) {
    List<Company> trn = new ArrayList();
    Company t = new Company();

    t.setName("Benz");
    t.setPrice(10000);
    trn.add(t);
    t = null;
    t = new Company();
    t.setName("BMW");
    t.setPrice(3000);
    trn.add(t);
    t = null;
    t = new Company();
    t.setName("Benz");
    t.setPrice(1000);
    trn.add(t);
    t = null;
    t = new Company();
    t.setName("Toyota");
    t.setPrice(500);
    trn.add(t);

    for (Company c : trn) {
        System.err.println(c.toString());
    }
    Map<String, String> map = new HashMap();
    int counter = 0;
    for (Company c : trn) {
        String name = c.getName();
        if (!map.containsKey(name)) {
            counter = 0;
            for (Company com : trn) {
                if (com.getName().equalsIgnoreCase(name)) {
                    counter += com.getPrice();
                }
            }
            map.put(name, String.valueOf(counter));
        }
    }
        System.err.println(map.get("Benz"));
        System.err.println(map.get("BMW"));
        System.err.println(map.get("Toyota"));
    }
}

Output

Company{name=Benz, price=10000}
Company{name=BMW, price=3000}
Company{name=Benz, price=1000}
Company{name=Toyota, price=500}
11000
3000
500

Your ArrayList and HashMap declarations are missing type arguments. Assuming if you're developing on Java 7 then all you're missing is just the diamond operator, e.g. ArrayList<>().

Company t = new Company();
t.setName("Benz");
t.setPrice(10000);
trn.add(t);
t = null;
t = new Company();
t.setName("BMW");
t.setPrice(3000);
trn.add(t);
t = null;
t = new Company();
t.setName("Benz");
t.setPrice(1000);
trn.add(t);
t = null;
t = new Company();
t.setName("Toyota");
t.setPrice(500);
trn.add(t);

This is an odd way of adding four objects into your List. Assuming if your Company object has the following simple constructor, you should do it as such:

trn.add(new Company("Benz", 10000));
trn.add(new Company("BMW", 3000));
trn.add(new Company("Benz", 1000));
trn.add(new Company("Toyota", 500));

Instead of a nested for loop and skipping entries that already exist in the result map, you can consider retrieving the existing calculated sum from the map, if any, and add the current price to that. Also, why is map created as Map<String, String> and not Map<String, Integer> or Map<Company, Integer>?

One more thing, you can consider putting this particular comparison as a method in your Company class: com.getName().equalsIgnoreCase(name).


squareStyle variable should be initialized in constructor instead of render. Reason being, render is called on each state update and so, if your render is called multiple times, the variable inside render will be computed again which will cause the existing colours of the digits to change on each and will be unpleasant to look.
Usage of var is replaced by let and consts

Modified Code

class Square extends React.Component {
  constructor(props) {
    super(props)
    this.squareStyle = {
      width: 25,
      height: 25,
      backgroundColor: props.colour || getRandomColor(),
      display: "inline-block"
    };
  }
  render() {
    return (
      <div style={this.squareStyle}>
      </div>
    );
  }
}


Simple game - hangman Recently I made simple hangman game in Java.

Main.java

package com.company;

import java.util.Arrays;
import java.util.Scanner;

public class Main {

    public static void main(String[] args) {

        Scanner input = new Scanner(System.in);

        System.out.println("Type a passwords");
        String word = input.nextLine();
        char[] password = word.toCharArray();
        char[] passwordView = new char[password.length];
        for (int w = 0; w < passwordView.length; w++) {
            passwordView[w] = '_';
        }

        int badAnswers = 0;
        char letter = 'x';
        String guess;
        boolean success;
        String answers = "";

        while (badAnswers < 11) {
            success = false;
            System.out.println("Type a char.");
            while (!success) {
                guess = input.nextLine();
                if (guess.length() == 1 && !(answers.contains(guess))) {
                    letter = guess.charAt(0);
                    success = true;
                    answers = answers + letter + ", ";
                } else if (!(guess.length() == 1)){
                    System.out.println("Type exactly one char!");
                }
                else if (answers.contains(guess)){
                    System.out.println("You have already typed that symbol!");
                }

            }

            CheckingIfPasswordContainsLetter checker = new CheckingIfPasswordContainsLetter();
            if (checker.cointans(password, letter)) {
                for (int i = 0; i < password.length; i++) {
                    if (password[i] == letter) {
                        passwordView[i] = letter;
                    }
                }
            } else {
                badAnswers++;
                System.out.println("Bad");
            }

            if (!(Arrays.equals(password, passwordView))) {
                System.out.println("Already used symbols: " + answers);
                System.out.print("Current password is ");
                new PrintingArray(passwordView);
                System.out.print("\n");
            }

            if (Arrays.equals(password, passwordView)) {
                System.out.print("You won! the password was ");
                new PrintingArray(password);
                break;
            }

            if (badAnswers == 11) {
                System.out.print("You lose! Sorry!");
                break;
            }
        }

    }
}

PrintingArray.java

package com.company;

public class PrintingArray {

    public PrintingArray(char[] a){
        for (char z : a) {
            System.out.print(z);
        }
    }

}

CheckingIfPasswordContainsLetter.java

package com.company;

public class CheckingIfPasswordContainsLetter {
    public boolean cointans(char[] a, char b) {
        for (int i = 0; i < a.length; i++) {
            if (a[i] == b) {
                return true;
            }
        }
        return false;
    }
}

for (int w = 0; w < passwordView.length; w++) {
            passwordView[w] = '_';
        }

There's a built-in for this

        Arrays.fill(passwordView, '_');

You don't have to do it manually.

        char letter = 'x';
        String guess;
        boolean success;

You could declare all these inside the loop instead by changing

            success = false;
            System.out.println("Type a char.");
            while (!success) {
                guess = input.nextLine();
                if (guess.length() == 1 && !(answers.contains(guess))) {
                    letter = guess.charAt(0);
                    success = true;
                    answers = answers + letter + ", ";

to

            char letter;
            boolean success = false;
            System.out.println("Type a char.");
            while (!success) {
                String guess = input.nextLine();
                if (guess.length() == 1 && !(answers.contains(guess))) {
                    letter = guess.charAt(0);
                    success = true;
                    answers = answers + letter + ", ";

or even better, get rid of success entirely.

            char letter;
            System.out.println("Type a char.");
            while (true) {
                String guess = input.nextLine();
                if (guess.length() == 1 && !(answers.contains(guess))) {
                    letter = guess.charAt(0);
                    answers = answers + letter + ", ";
                    break;

or consider something like

            char letter = inputLetter(input, answers);

with a properly defined method.

                new PrintingArray(passwordView);
                System.out.print("\n");

You don't need PrintingArray, but you could just say

                System.out.println(passwordView);

and get the same effect.

            CheckingIfPasswordContainsLetter checker = new CheckingIfPasswordContainsLetter();
            if (checker.cointans(password, letter)) {

You don't need this either.  You could just say

            int firstOccurrence = word.indexOf(letter);
            if (firstOccurrence >= 0) {
                for (int i = firstOccurrence; i < password.length; i++) {

This also has the side effect of not duplicating the scan.  Note that the original version started over at the beginning.


Private VBA Class Initializer called from Factory I've been playing with COM lately and while getting to understand the mechanism of how class methods/properties are called an idea came to mind: what if we can have a global instance of a class that exposes a Factory for creating new instances of that class but the Initializer method is Private. Is that possible? The answer is YES. We can make use of the Me special variable to find and replace the instance pointer so that we can redirect calls to the desired instance.
Consider Class1 which has the VB_PredeclaredId set to True:
VERSION 1.0 CLASS
BEGIN
  MultiUse = -1  'True
END
Attribute VB_Name = "Class1"
Attribute VB_GlobalNameSpace = False
Attribute VB_Creatable = False
Attribute VB_PredeclaredId = True
Attribute VB_Exposed = False
'@PredeclaredId
Option Explicit

#If Mac Then
    #If VBA7 Then
        Private Declare PtrSafe Function CopyMemory Lib "/usr/lib/libc.dylib" Alias "memmove" (Destination As Any, Source As Any, ByVal Length As LongPtr) As LongPtr
    #Else
        Private Declare Function CopyMemory Lib "/usr/lib/libc.dylib" Alias "memmove" (Destination As Any, Source As Any, ByVal Length As Long) As Long
    #End If
#Else 'Windows
    'https://msdn.microsoft.com/en-us/library/mt723419(v=vs.85).aspx
    #If VBA7 Then
        Private Declare PtrSafe Sub CopyMemory Lib "kernel32" Alias "RtlMoveMemory" (Destination As Any, Source As Any, ByVal Length As LongPtr)
    #Else
        Private Declare Sub CopyMemory Lib "kernel32" Alias "RtlMoveMemory" (Destination As Any, Source As Any, ByVal Length As Long)
    #End If
#End If

#If Win64 Then
    Private Const PTR_SIZE As Long = 8
#Else
    Private Const PTR_SIZE As Long = 4
#End If

Private m_name As String
Private m_id As Long

Public Function Factory(ByVal newName As String, ByVal newID As Long) As Class1
    Dim newClass1 As Class1
    Set newClass1 = New Class1
    '
    #If VBA7 Then
        Dim mePtr As LongPtr
        Dim swapAddr As LongPtr
    #Else
        Dim mePtr As Long
        Dim swapAddr As Long
    #End If
    '
    'Find the address where the swap must happen
    'Note we cannot save ObjPtr(Me) to a variable because
    '   we could get the position of that variable instead
    swapAddr = VarPtr(Me)
    Do
        swapAddr = swapAddr + PTR_SIZE
        CopyMemory mePtr, ByVal swapAddr, PTR_SIZE
    Loop Until mePtr = ObjPtr(Me)
    'Debug.Print swapAddr - VarPtr(Me) '56 on x64 and 168 on x32
    '
    CopyMemory ByVal swapAddr, ObjPtr(newClass1), PTR_SIZE
    Init newName, newID
    CopyMemory ByVal swapAddr, mePtr, PTR_SIZE
    '
    Set Factory = newClass1
 End Function

Private Sub Init(ByVal newName As String, ByVal newID As Long)
    m_name = newName
    m_id = newID
End Sub

Public Property Get Name() As String
    Name = m_name
End Property

Public Property Get ID() As Long
    ID = m_id
End Property

Now we could create and use new instances like this:
Sub TestFactory()
    With Class1.Factory("Test", 4)
        Debug.Print .Name
        Debug.Print .ID
    End With
End Sub

even if the Init method is Private.
I don't really understand why the offset is 56 bytes on x64 and 168 bytes on x32 (at least on my computers). Would be nice if somebody could figure this out so that the loop used in finding the swap address is not needed anymore.

EDIT 1
Apparently on x64 it is sufficient to get the swap address like this:
    #If Win64 Then
        swapAddr = VarPtr(Factory) + PTR_SIZE
        mePtr = ObjPtr(Me)
    #End If

so no loop would be needed.
Edit 2
I've decided to create a new follow-up question with a new improved code, instead of answering this question because the code here is slower and less safer. Go to: Private VBA Class Initializer called from Factory #2

This is not so much an review but a variation of the OP's technique.   As  Mathieu Guindon mentioned in the comments the "logic needs to be encapsulated".  My Caster class does just that.  The Person class is just for testing.
Caster:Class
Attribute VB_Name = "Caster"
Option Explicit

#If Mac Then
    #If VBA7 Then
        Private Declare PtrSafe Function CopyMemory Lib "/usr/lib/libc.dylib" Alias "memmove" (Destination As Any, Source As Any, ByVal Length As LongPtr) As LongPtr
    #Else
        Private Declare Function CopyMemory Lib "/usr/lib/libc.dylib" Alias "memmove" (Destination As Any, Source As Any, ByVal Length As Long) As Long
    #End If
#Else                                            'Windows
    'https://msdn.microsoft.com/en-us/library/mt723419(v=vs.85).aspx
    #If VBA7 Then
        Private Declare PtrSafe Sub CopyMemory Lib "kernel32" Alias "RtlMoveMemory" (Destination As Any, Source As Any, ByVal Length As LongPtr)
    #Else
        Private Declare Sub CopyMemory Lib "kernel32" Alias "RtlMoveMemory" (Destination As Any, Source As Any, ByVal Length As Long)
    #End If
#End If

#If Win64 Then
    Private Const PTR_SIZE As Long = 8
#Else
    Private Const PTR_SIZE As Long = 4
#End If

#If VBA7 Then
    Private SourcePointer As LongPtr
    Private DestinationPointer As LongPtr
#Else
    Private SourcePointer As Long
    Private DestinationPointer As Long
#End If

Public Sub SaveAs(ByRef Source As Object, ByRef Destination As Object)
    DestinationPointer = VarPtr(Source)
    Do
        DestinationPointer = DestinationPointer + PTR_SIZE
        CopyMemory SourcePointer, ByVal DestinationPointer, PTR_SIZE
    Loop Until SourcePointer = ObjPtr(Source)

    CopyMemory ByVal DestinationPointer, ObjPtr(Destination), PTR_SIZE
End Sub

Public Sub Restore()
    CopyMemory ByVal DestinationPointer, SourcePointer, PTR_SIZE
End Sub

Person:Class
Note:  The Init method is just to demonstrate the OP's original concept.  It could be replaced with  m = t.
Attribute VB_Name = "Person"
Attribute VB_PredeclaredId = True

Option Explicit

Private Type Members
    DOB As Date
    Name As String
    Sex As String
End Type

Private this As Members

Public Function Factory(pDOB As Date, pName As String, pSex As String) As Person
    DOB = pDOB
    Name = pName
    Sex = pSex
    Set Factory = Clone
End Function

Public Function Clone() As Person
    Dim Object As Person
    Set Object = New Person

    Dim Caster As New Caster

    Dim t As Members
    t = this
    Caster.SaveAs Me, Object
    Init t
    Caster.Restore
    Set Clone = Object
End Function

Private Sub Init(t As Members)
    this = t
End Sub

Public Property Get DOB() As Date

    DOB = this.DOB

End Property

Public Property Let DOB(ByVal Value As Date)

    this.DOB = Value

End Property

Public Property Get Name() As String

    Name = this.Name

End Property

Public Property Let Name(ByVal Value As String)

    this.Name = Value

End Property

Public Property Get Sex() As String

    Sex = this.Sex

End Property

Public Property Let Sex(ByVal Value As String)

    this.Sex = Value

End Property

Test
Sub TestClone()
    Dim Tom As New Person
    With Tom
        .DOB = #7/26/1970#
        .Name = "Tom"
        .Sex = "M"
    End With

    Debug.Print "TestClone"
    With Tom.Clone
        Tom.Name = "Thomas"
        Debug.Print "DOB: ", .DOB
        Debug.Print "Name: ", .Name
        Debug.Print "Sex: ", .Sex
    End With
    Debug.Print
End Sub

Sub TestFactory()
    Dim Tom As Person
    Set Tom = Person.Factory(#7/26/1970#, "Tom", "M")
    Person.Name = "Thomas"
    Debug.Print "TestFactory"
    With Tom
        Debug.Print "DOB: ", .DOB
        Debug.Print "Name: ", .Name
        Debug.Print "Sex: ", .Sex
    End With
End Sub


Number Guessing Game in Java This is my implementation of the famous Number Guessing Game. It is not object oriented, but the goal was to create a simple to read, procedural program. Have I accomplished this goal?

import java.util.Random;
import java.util.Scanner;

class Main {
  private static int lowerRange = 1;
  private static int upperRange = 99;
  private static int tries = 6;

  private static Scanner input = new Scanner(System.in);
  private static Random random = new Random();

  public static void main(String[] args) {
    showDescription();
    boolean run = true;
    while (run) {
      play();
      System.out.print("Play again? (Y / N): ");
      run = input.nextLine().equalsIgnoreCase("Y") ? true : false;
    }
  }

  private static void showDescription() {
    System.out.println("You have to guess a number between " + lowerRange + " and " + upperRange + ".");
    System.out.println("You have " + tries + " tries.");
  }

  private static void play() {
    int randomNumber = generateRandomNumber(lowerRange, upperRange);
    int triesSoFar = 0;
    boolean won = false;
    while (triesSoFar < tries && won == false) {
      System.out.print("Your try: ");
      int guess = Integer.parseInt(input.nextLine());
      if (guess > randomNumber) {
        System.out.println("The secret number is smaller.");
      } else if (guess < randomNumber) {
        System.out.println("The secret number is higher.");
      } else {
        won = true;
        break;
      }
      triesSoFar++;
    }

    if (won) {
      System.out.println("You found the secret number!");
    } else {
      System.out.println("You lose. The secret number was " + randomNumber + ".");
    }
  }

  private static int generateRandomNumber(int lowerRange, int upperRange) {
    return random.nextInt(upperRange - lowerRange) + lowerRange;
  }
}

You have showDescription directly printing out the message. Avoid printing in arbitrary functions whenever possible. It's much better to have things return Strings, then print at the call site as needed. I'd change that function to:

private static String produceDescription() {
    return "You have to guess a number between " + lowerRange + " and " + upperRange + ".\n"
           + "You have " + tries + " tries.");
}

. . .

System.out.println(produceDescription());

Why? Two highly-related reasons:

Whenever possible, functions should return the data that they produce. Can you guarantee that for a function like this you will always want to directly print that data? Let the caller decide how they want to use the data that the function produces. Forcing the data to be printed makes the function less useful in the long term. As an example...
If you ever decide to adapt this program to use a full GUI, you're directly printing and will need to modify every function that is calling println. The less functions you have that are using data in a specific way, the easier it will be to alter your program later.

I would not make everything static here. Again, what if in the future you wanted to run two games at the same time (like if you created a server that allows people to connect to it and play)? I'd get rid of static everywhere, and make everything plain instance methods/properties, then just instantiate a instance of the game in main.

Main is a poor name for this class. Ideally, it should be a description of what the object accomplishes. What if you ever imported this class so it can be used elsewhere? A class called Main doesn't make it immediately obvious what it is used for. I'd change the name to something like NumberGuessingGame.

run = input.nextLine().equalsIgnoreCase("Y") ? true : false;

is redundant. Ternary expressions are cool, but they're often overused. Think about it, what does equalsIgnoreCase return? A bool (true or false). You're then using that bool as a condition to the ternary... to get the same thing that equalsIgnoreCase returned originally.

Just get rid of the ternary:

run = input.nextLine().equalsIgnoreCase("Y");

And on the topic of conditions, a little later you have:

while (triesSoFar < tries && won == false) {

won == false certainly isn't wrong, but comparing against a boolean value directly is almost always unnecessary. Just write:

while (triesSoFar < tries && !won) {

Remember, ! is read as "not". "While tries to far is less than tries, and they haven't (not) won".

tries is a bad name. It isn't representing the number of tries taken (that's triesSoFar), it's representing the max number of tries allowed. Change it to something like maxAllowedTries.

Naming is very important. It is one of the key things that allows someone to be able to read your code and quickly know what's going on. Make sure your functions and variable names accurately describe what they do, or you'll make other people's lives more difficult when they need to read your code. You'll also make your own life more difficult if you ever come back to this program, because you will eventually forget parts of this program after some time.

System.out.print("Your try: ");

Isn't actually guaranteed to print right away. print and println use a "buffer" to hold text while waiting to print. That buffer is only "flushed" (printed out) when the text exceeds a certain length, or a newline (\n) is reached. You have short text here, and because you're using print, no newline is being added to the buffer. You may find that if you made this text a little shorter, it wouldn't print until some other text had be printed as well, which will make your program confusing.

If you use print instead of println, it can be a good idea to add a call to System.out.flush() after it to make sure everything is printed when you want it to be.

Just to show a potentially appropriate use of a ternary, you have:

if (won) {
  System.out.println("You found the secret number!");
} else {
  System.out.println("You lose. The secret number was " + randomNumber + ".");
}

Which isn't bad, but it could be shorted a bit:

String message = won ? "You found the secret number!"
                     : ("You lose. The secret number was " + randomNumber + ".");

System.out.println(message);

I'm not necessarily advocating for this way, but I thought I'd show it. It allows you to get rid of the multiple calls to println.

  int guess = Integer.parseInt(input.nextLine());
  if (guess > randomNumber) {
    System.out.println("The secret number is smaller.");
  } else if (guess < randomNumber) {
    System.out.println("The secret number is higher.");
  } else {
    won = true;
    break;
  }

is a dense chunk of code. I prefer to add more spacing around things. I like blank lines above ifs and elses so it's easier to see at a glance the distinct blocks. I'd make it:

  int guess = Integer.parseInt(input.nextLine());

  if (guess > randomNumber) {
    System.out.println("The secret number is smaller.");

  } else if (guess < randomNumber) {
    System.out.println("The secret number is higher.");

  } else {
    won = true;
    break;
  }

That generally makes it easier to pick things out by eye when scanning over a document.

There's some more stuff, but unfortunately, I have to go. Good luck!


Textbased User Interface for user and program taking turns I wrote this program to model interactions between a user and an artificial player, both playing by the same rules (not enforced here for simplicity).
The game played is here is "your next word has to start with the last letter of mine"

module Main where

import Lib
import System.Random
import System.Exit
import Control.Monad

vocab     = ["alpha","beta","gamma"]
blacklist = []

pick:: [a] -> IO a --picks random element. copy pasted, not understood
pick x = Control.Monad.liftM (x !!) (randomRIO (0, length x - 1))

main :: IO ()
main = do
  userInput <- getLine
  processUser userInput vocab blacklist

processUser :: String -> [String] -> [String] -> IO a
processUser input vocab blacklist = if input == "quit" then exitSuccess
                                    else do
                                          successor <- getNext input vocab (input:blacklist)
                                          processPC successor vocab blacklist

processPC :: Maybe (IO String) -> [String] -> [String] -> IO a
processPC  Nothing      v b = do putStrLn "I give up"
                                 exitSuccess

processPC (Just ioWord) v b = do word <- ioWord
                                 putStrLn word
                                 userInput <- getLine
                                 processUser userInput v (word:b)

getNext:: String -> [String] -> [String] -> IO (Maybe (IO String))
getNext lastWord vocab blacklist  = do let chooseFrom = filter (`notElem` blacklist) vocab
                                       let matches    = filter (\x -> head x == last lastWord) chooseFrom
                                       if null matches then return Nothing
                                       else return (Just (pick matches) )

I am especially interested how to formulate this better structurally-

The very first thing that jumps out at me is the indirection of maintaining both a list of possible words and a seen list. Picking a better representation for your state will make bookkeeping easier.

type Word = String
type Words = [Word]
type Moves = Map.Map Char Words

Now fill in the logically necessary functions needed to work with that state. We need to be able to construct a state blob—

makeMoves :: Words -> Moves
makeMoves = Map.fromListWith (++) . catMaybes . map tag
  where
    tag :: Word -> Maybe (Char, Words)
    tag []      = Nothing
    tag w@(c:_) = Just (c, [w])

Remove played words from one—

remove :: Word -> Moves -> Moves
remove []      = id
remove w@(c:_) = Map.adjust (delete w) c

And choose a new word from one given the constraint imposed by the last play.

move :: Char -> Moves -> IO (Maybe (Word, Moves))
move c ms =
  let possible = Map.lookup c ms
  in  case possible of
        Nothing -> return Nothing
        Just [] -> return Nothing
        Just ws -> do
          w <- pick ws
          return $ Just (w, remove w ms)

Note the separation from any logic in taking turns, or which player is currently up. Testing these operations will be much easier then (load them up in GHCi and try it out), and integrating them into our monadic game playing code should be pretty straightforward, just follow the types.

processUser :: Moves -> IO ()
processUser moves = do
    input <- getLine
    case input of
      "quit" -> return ()
      ""     -> do putStrLn "You must enter a word."
                   processUser moves
      w      -> processPC (last w) (remove w moves)

Note also how I have moved the logic for making a player move into the function where it makes sense to do so. User player code shouldn't drive computer player code, and vice versa. As an exercise, try making the necessary modifications and stylistic tweaks to processPC yourself.

You should make a best effort attempt at understanding all of the functions you write into your source. Copy and pasting pick in when you don't understand it is poor form, where'd you even find that definition? At least use a library. If you don't know what liftM is doing, just write the function out using do-notation as you would any other operation in the IO monad.

pick :: [a] -> IO a
pick xs = do
    i <- randomRIO (0, length xs - 1)
    return (xs !! i)

Spoilers—Here are the commit-level changes and final runnable code I produced while working through this.


Function that returns value based on specific category This is a pretty simple function that returns a value based on the variable category. I tried to use switch instead of define menu object that contained functions with tag name but I noticed that it looks pretty dirty so that's how I made this code. After that I realized defining menu variable will run every function in the object to store the value in the properties which is not what I'm trying to do. What I want to do is that I want to run only a function and it needs to run to get a value based on the variable category. How could I make this better?

const getContent = async (category, tag) => {
    let query = { 'tag':tag, 'limit':10 }

    let menu = {
        'feed': () => steem.api.getDiscussionsByFeed(query),
        'blog': () => steem.api.getDiscussionsByBlog(query),
        'new': () => steem.api.getDiscussionsByCreated(query),
        'hot': () => steem.api.getDiscussionsByHot(query),
        'trend': () => steem.api.getDiscussionsByTrending(query)
    }

    return await menu[category]()
}

After that I realized defining menu variable will run every function in the object to store the value in the properties which is not what I'm trying to do.

What you have looks like it would work just fine.  Declaring the menu structure does not execute the functions you've assigned as properties.  It just defines the functions and assigns them as properties (waiting to be called later).

You could save a little execution overhead by moving the static menu definition outside the function so it just gets defined once (I'm assuming this is private module scope so moving the symbol outside the function is fine) and change it to const.

You can also remove the async and await since neither add anything here.  You're just returning a promise anyway so you can just return the promise directly without async and await.  I also added more explicit error checking for the category name.

const methodMap = {
    'feed': () => steem.api.getDiscussionsByFeed(query),
    'blog': () => steem.api.getDiscussionsByBlog(query),
    'new': () => steem.api.getDiscussionsByCreated(query),
    'hot': () => steem.api.getDiscussionsByHot(query),
    'trend': () => steem.api.getDiscussionsByTrending(query)
};

const getContent = (category, tag) => {
    const query = { 'tag':tag, 'limit':10 };
    if (methodMap.hasOwnProperty(category)) {
        return methodMap[category]();
    } else {
        return Promise.reject(new Error(`Unknown Category: ${category}`));
    }
}

You could also remove some redundancy from the table by making the table only be the steem method name you want to call.

const methodMap = {
    'feed': 'getDiscussionsByFeed',
    'blog': 'getDiscussionsByBlog',
    'new': 'getDiscussionsByCreated',
    'hot': 'getDiscussionsByHot',
    'trend': 'getDiscussionsByTrending'
};

const getContent = (category, tag) => {
    const query = { 'tag':tag, 'limit':10 };
    if (methodMap.hasOwnProperty(category)) {
       return steem.api[methodMap[category]](query);
    } else {
        return Promise.reject(new Error(`Unknown Category: ${category}`));
    }
}

This layout also does more explicit error checking on a "supported" category being passed in.


The Palindromic Odometer Puzzler: A Programmatic Solution That is a programmatic solution to a puzzler from Car Talk, its summary is:

  "I noticed that the last 4 digits were palindromic. I drove a mile,
  and the last 5 were palindromic. I drove another mile and the middle 4
  were palindromic, and the ends were not involved. And then one mile
  later, all 6 digits were palindromic."

def is_palindrome(string):
    """Finds if a string is a plaindrome or not.

       string: str

       returns: bool
       """
    return string == string[::-1]

def find_puzzle_number():
    """Finds the number which was first seen.

       prints: int
       """
    guess = 100000
    # the odometer shows only 6 numbers,
    # therefore 999999 is the last number to test.
    while guess < 1000000:
        if is_palindrome(str(guess)[2:]):  # last 4 numbers.
            if is_palindrome(str(guess + 1)[1:]):  # last 5 numbers.
                if is_palindrome(str(guess + 2)[1:5]):  # middle 4 numbers.
                    if is_palindrome(str(guess + 3)):
                        print(guess)

        guess = guess + 1

print('These are the possible odometer readings: ')
find_puzzle_number()

Notes:

I'm a hobbyist and a beginner.
I don't only want to optimize the code, but also my skills and style. Therefore any notes, advice and suggestions aren't only welcomed, but also encouraged!

The documentation

   string: str

   returns: bool

is pretty obvious. I would skip it. The rest of the documentation can be more idiomatically written as

"""Return whether input sequence is a palindrome."""

(note palindrome, not plaindrome). I removed the term string because many sequence types can be palindromic, like [1, 2, 3, 2, 1].

find_puzzle_number should yield, not print, its values.

I would change the documentation to specify the rules the output satisfy.

def find_puzzle_number():
    """Generates 6-digit integers, x, where all of

    * the last 4 digits of x,
    * the last 5 digits of x+1,
    * the middle 4 digits of x+2, and
    * all 6 digits of x+3

    are palindromes.
    """
    guess = 100000
    # the odometer shows only 6 numbers,
    # therefore 999999 is the last number to test.
    while guess < 1000000:
        if is_palindrome(str(guess)[2:]):  # last 4 numbers.
            if is_palindrome(str(guess + 1)[1:]):  # last 5 numbers.
                if is_palindrome(str(guess + 2)[1:5]):  # middle 4 numbers.
                    if is_palindrome(str(guess + 3)):
                        yield guess

        guess = guess + 1

The while should become a for

# 6 digit numbers
for guess in range(10**5, 10**6):

Then the if chain of is_palindromes can be simplified:

valid = (
    is_palindrome(str(guess + 0)[2:]) and
    is_palindrome(str(guess + 1)[1:]) and
    is_palindrome(str(guess + 2)[1:5]) and
    is_palindrome(str(guess + 3))
)

if valid:
    yield guess

Now note something bad: guess + 3, can overflow. Either you want to clamp the code so that guess + 3 < 10**6, or you want to perform modular arithmetic.

Further, you don't know that 10**5 is a valid lower bound: 000000 is a valid reading. You can use format(guess, "06") to pad like this.

If you care about speed, PyPy makes the code run in about a tenth the time.

def is_palindrome(seq):
    """Return whether input sequence is a palindrome."""
    return seq == seq[::-1]

def find_puzzle_number():
    """Generates 6-digit integers, x, where all of

    * the last 4 digits of x,
    * the last 5 digits of x+1,
    * the middle 4 digits of x+2, and
    * all 6 digits of x+3

    are palindromes.
    """
    def as_reading(x):
        return format(x % 10**6, "06")

    # 6 digit numbers
    for guess in range(10**6):
        valid = (
            is_palindrome(as_reading(guess + 0)[2:]) and
            is_palindrome(as_reading(guess + 1)[1:]) and
            is_palindrome(as_reading(guess + 2)[1:5]) and
            is_palindrome(as_reading(guess + 3))
        )

        if valid:
            yield guess

print('These are the possible odometer readings: ')
for solution in find_puzzle_number():
    print(solution)

Note that there are simple optimizations you can apply. For instance, you can easily iterate over only those values for which guess + 3 is a palindrome by construction - this reduces the values to iterate over from \$10^6\$ to \$10^3\$.

for guess_digits in range(10**3):
    # guess + 3 is a palindrome
    # NOTE: relies on correct handling of wrapping
    guess = guess_digits + int(format(guess_digits, "06")[::-1]) - 3

This makes the code basically instantaneous, so if optimizations were pointless to apply before they're most certainly pointless to apply now.


A better cryptography process I am experimenting with cryptography and I was wondering if somebody has some suggestion how I could improve my code and methodology.

I want my process to generate more securely the cryptography process if that is possible

import java.security.GeneralSecurityException;
import java.security.Key;
import java.util.Base64;
import javax.crypto.Cipher;
import javax.crypto.spec.SecretKeySpec;

@SuppressWarnings("ClassWithoutLogger")
public class Crypto_Security {

  static public final String CRY_ALGO = "AES";

  static public String encrypt(byte[] pSecret, String pValue) throws GeneralSecurityException {
    Key aesKey = new SecretKeySpec(pSecret, CRY_ALGO);
    Cipher cipher = Cipher.getInstance(CRY_ALGO);
    cipher.init(Cipher.ENCRYPT_MODE, aesKey);
    byte[] encrypted = cipher.doFinal(pValue.getBytes());
    Base64.Encoder encoder = Base64.getEncoder();
    return encoder.encodeToString(encrypted);
  }

  static public String decrypt(byte[] pSecret, String pEncrypted) throws GeneralSecurityException {
    SecretKeySpec skeySpec = new SecretKeySpec(pSecret, CRY_ALGO);
    Cipher cipher = Cipher.getInstance(CRY_ALGO);
    cipher.init(Cipher.DECRYPT_MODE, skeySpec);
    byte[] original = cipher.doFinal(Base64.getDecoder().decode(pEncrypted));
    return new String(original);
  }

  static public void main( String[] args ) {
    try {
      System.out.println("enc " + encrypt( "2144226321110063".getBytes() , "serial.txt" ));
      System.out.println("dcr " + decrypt( "2144226321110063".getBytes() , "oLQgEUxg3Z3zMSsSkiKpBg==" ));
    } catch( GeneralSecurityException e ) {
      e.printStackTrace();
    }
    return;
  }

  private Crypto_Security() {
  }

}

There really isn't all that much to see here when it comes to cryptography. Basically this is a wrapper class that tries to wrap the knowledge of the person that wrote it (that's you I suppose) rather than to implement security for a specific use case. In such a sense, it is only useful for practice purposes.

Never ever create such a class and make it the center of your security strategy (I've been there, steered there by "professionals", took me weeks to recover from it a couple of years later).

I'll just focus on the security parts:

a string is not a key, if you want to use a password, use a Password Based Key Derivation Mechanism such as PBKDF2 (and keep in mind that most passwords are insecure even if you do take that measure - a number is certainly not a great password);
just "AES" will default to the insecure ECB mode for most if not all Java / provider implementations - it is OK for the key creation, but not for the "transformation" parameter in Cipher.getInstance(String transformation): Cipher;
all other modes require an IV for generic messages, as otherwise identical messages (or parts of messages) may leak data - and even all data in the case of counter mode encryption;
nowadays authenticated ciphers such as GCM should be used; this makes sure that an adversary cannot alter the ciphertext undetected by the receiver - such alterations may even threaten the confidentiality of the message (see plaintext / padding oracle attacks);
using pValue.getBytes() is dangerous as getBytes will default to platform encoding, and the platform encoding may differ per platform (Windows uses Windows-1252 and Linux / Android uses UTF-8, to name just one such difference) - try e.g. pValue.getBytes(StandardCharsets.UTF_8) instead;
just throwing security exceptions doesn't make sense, as it doesn't distinguish between runtime issues (algorithm not available) and input / output issues - see here how to handle security exceptions;

The first thing I do in my IDE is to change e.printStackTrace() to new IllegalStateException("No exception yet specified", e);, usually with a // TODO change exception comment in front of it (picked up by the task system in Eclipse).

The problems with just printing the stack trace is that it is easily forgotten, it keeps running the code even after an exception is turned up and finally, it messes up the compilers detection of dead code.

CRY_ALGO of course is a cry-out for a better name.


My Binary Search Implementation I have implemented binary search in JavaScript.

I run it on node.

I pass an comma separated string of numbers in ascending order as first argument and the number to be searched in the second argument.

the code is as follows,

var myArray = [],
searchNum = undefined;

// below function is used to capture the
// commandline parameters for array and the
// number to be searched
(function(){
    process.argv.forEach(function (val, index, array) {
        var idx = 0, ar = undefined;

        try{

            if(index === 2){

                ar = val.split(",");

                // convert the numbers present as string values is array
                // to array of numbers
                for(idx = 0; idx < ar.length; idx++){
                    myArray.push(parseInt(ar[idx]));
                }
            }// end of if

            // third index is the number to be searched.
            if(index === 3){
                searchNum = parseInt(val)
            }

        }catch(e){
            console.log(e)
        }

    });
})();

console.log(" SEARCH NUMBER ",searchNum," in array ",myArray);
console.log(" Number ",searchNum," "+binarySearch(myArray,searchNum));

// binary-Search implementation
function binarySearch(myArray,numberToSearch){
    var arrayLength = myArray.length,
        midIndex = parseInt(arrayLength/2),
        subArray = undefined,
        lowerIndex = 0,
        higherIndex = 0;

    if(myArray[midIndex] === numberToSearch){
        return "is Found";

    // search the number in left side if array
    // i.e. number is found to the left of the
    // middle Index
    }else if(midIndex !== 0 && myArray[midIndex] > numberToSearch){
        lowerIndex = 0;
        higherIndex = midIndex;
        subArray = myArray.slice(lowerIndex,higherIndex); // create the sub-array
        return binarySearch(subArray,numberToSearch); // search the number in the subarray

    // search the number in right side if array
    // i.e. number is found to the right of the
    // middle Index
    }else if(midIndex !== 0 && myArray[midIndex] < numberToSearch){ // search the number in right side if array
        lowerIndex = midIndex + 1;
        higherIndex = arrayLength;
        subArray = myArray.slice(lowerIndex,higherIndex); // create the sub-array
        return binarySearch(subArray,numberToSearch); // search the number in the subarray
    }else{
        return "can't be found";
    }
}// end of binarySearch method

I run the above as follows

E:\RahulShivsharan\MyPractise\DesignPatternsInJavaScript>node binarySearch.js 34,45,67,89,90,123,345 300
 SEARCH NUMBER  300  in array  [ 34, 45, 67, 89, 90, 123, 345 ]
 Number  300  can't be found

E:\RahulShivsharan\MyPractise\DesignPatternsInJavaScript>node binarySearch.js 34,45,67,89,90,123,345 45
 SEARCH NUMBER  45  in array  [ 34, 45, 67, 89, 90, 123, 345 ]
 Number  45  is Found

E:\RahulShivsharan\MyPractise\DesignPatternsInJavaScript>

Requesting to please review my above implementation, and give your valuable feedback.

Just focusing on the initialization part.

The code is using both index and idx which gives off a code smell, something could be better there
The code is using idx just to loop over an array, apply a function and push. We can do better than this
Comments should be useful, commenting the end of an if statement is not useful

(function(){
  process.argv.forEach(function (val, index, array) {

    try{
      //Add the comma separated numbers to myArray as integers
      if(index === 2){
        myArray = myArray.concat( val.split(",").map(c=>parseInt(c)) );
      } else if (index === 3){
        //Third index is the number to be searched.
        searchNum = parseInt(val)
      }
    }catch(e){
      console.log(e)
    }
  });
})();

If you think further, then we could skip the whole loop business, and just access the 2 values we need. It would be simpler, and better.

    (function(){
      var args = process.argv;

      try{
        //Add the comma separated numbers to list as integers
        list = list.concat( args[2].split(",").map(c=>parseInt(c)) );
        //Third index is the number to be searched.
        searchNum = parseInt( args[3] );
      }catch(e){
        console.log(e)
      }
    })();

myArray is not a great name, I preferred list.


Updating XML config files I have been unable to find any other way to update a user config file than the following method.

public class SettingServices : ISettingServices
{
    public UserSetting GetUserSetting()
    {
        XmlSerializer reader = new XmlSerializer(typeof(UserSetting));
        StreamReader file = new StreamReader(UserConfig);
        UserSetting settings = new UserSetting();
        settings = (UserSetting)reader.Deserialize(file);
        return settings;
    }
    public void CreateUserSetting(UserSetting userSetting)
    {
        XmlSerializer writer = new XmlSerializer(typeof(UserSetting));
        StreamWriter file = new StreamWriter(UserConfig);
        writer.Serialize(file, userSetting);
        file.Close();
    }
    public void UpdateUserSetting(UserSetting userSetting)
    {
        UserSetting currentSettings = GetUserSetting();
        UserSetting newSetting = new UserSetting();
        newSetting.Name = (userSetting.Name == null) ? currentSettings.Name : userSetting.Name;
        newSetting.ViewChangeLog = (userSetting.ViewChangeLog == null) ? currentSettings.ViewChangeLog : userSetting.ViewChangeLog;
        newSetting.PrimaryRowColor = (userSetting.PrimaryRowColor == null) ? currentSettings.PrimaryRowColor : userSetting.PrimaryRowColor;
        newSetting.SecondaryRowColor = (userSetting.SecondaryRowColor == null) ? currentSettings.SecondaryRowColor : userSetting.SecondaryRowColor;
        newSetting.Font = (userSetting.Font == null) ? currentSettings.Font : userSetting.Font;
        CreateUserSetting(newSetting);
    }
    private static string UserConfig
    {
        get
        {
            string configDirectory = new Uri(Environment.GetFolderPath(Environment.SpecialFolder.ApplicationData) + "\\CVS\\ICMQuery").LocalPath;
            return new Uri(configDirectory + "\\" + System.Environment.UserName + ".config").LocalPath;
        }
    }
}

Can someone please show me the light. I would preferably like to be able to update singular elements within the UserSetting class without destroying and rewriting the file every time I have to update it.

Preferably I'd like to just be able to update that node in the XML file without having to rewrite the whole file every time a change is made.

In general, that's simply not possible, because XML is a text-based format.

Imagine you have the following file:

<UserSettings>
    <Name>Aron</Name>
    <!-- other settings here -->
</UserSettings>

Now you want to fix the name from “Aron” to “Aaron”:

<UserSettings>
    <Name>Aaron</Name>
    <!-- other settings here -->
</UserSettings>

But doing this means that everything that's after the Name line has to be moved by one byte in the file, so almost the whole file has to be rewritten.

With config files, this usually doesn't matter, because they tend to be relatively small. If it does matter to you, you will have to use another way of saving the config; a good choice might be an embedded database, like SQL Server Compact or SQLite.

Yet another option would be to use .settings files (built-in in VS). It doesn't save you from rewriting the file, but it saves you from having to write all that code.


User Favorites Service I'm practicing my OOP and so created this service that stores a user's favorites from an application.

The favorite has an ID, a owner ID, and his type. This service only saves and makes this information available.

When instantiating the service, must be provided a API KEY and the types of favorites available.

class FavoritesService {

  /**
   * string
   */
  private $key;

  /**
   * array
   */
  private $data;

  /**
   * array
   */
  private $types;

  /**
   * @param string $key The key of the application.
   * @param array $types Array containing the type of favorites.
   */
  public function __construct($key, $types) {
    $this->key = $key;
    $this->types = $types;
  }

  /**
   * Saves a favorite.
   *
   * @param integer $id The ID of the favorite.
   * @param integer $ownerId The ID of his owner.
   *
   * @throws InvalidArgumentException If the provided argument is not a valid type.
   * @throws Exception If the save to somewhere goes wrong.
   *
   * @return boolean Indicating that all went fine.
   */
  public function save($id, $ownerId, $type) {
    if (!in_array($type, $this->types)) {
        throw new InvalidArgumentException('Invalid favorite type.');
    }
    $this->data[$ownerId][] = self::buildResult($id, $type);
    // also save everything to somewhere
    return true;
  }

  /**
   * Provides all the favorites of a owner.
   *
   * @param integer $ownerId The ID of the owner.
   *
   * @return array|null All the favorites from the user or, if the user doesn't exists, null.
   */
  public function get($ownerId) {
      if (isset($this->data[$ownerId])) {
        return $this->data[$ownerId];
      }
      // checks somewhere if the data exists
      return null;
  }

  /**
   * Resets all the favorites of a owner.
   *
   * @param integer $ownerId The ID of the owner.
   *
   * @throws Exception If the reset to somewhere goes wrong.
   *
   * @return boolean Indicating that all went fine.
   */
  public function reset($ownerId) {
    unset($this->data[$ownerId]);
    // also update everything to somewhere
    return true;
  }

 /**
  * Builds a result of a favorite.
  *
  * @param integer $id The ID of the favorite.
  * @param string $type The type of the favorite.
  *
  * @return array An array with the favorite.
  */
  private static function buildResult($id, $type) {
    return [
        "id" => $id,
        "type" => $type
    ];
  }
}

An usage example. Users favorite tourism Places.

$favoritesService = new FavoritesService(
  'MY_SECRET_API_KEY',
  ['HOTEL', 'REGION', 'POI']
);

$favoritesService->save(1, 123, 'HOTEL');
$favoritesService->save(1, 321, 'REGION');
print_r($favoritesService->get(1));

$favoritesService->reset(1);
print_r($favoritesService->get(1));

My question, is this a good OOP design?

Is there a better naming than Service? It seems to generic to me.

I would agree with your concern over naming, as this doesn't really seems to be a "service", but rather a simple data store.  So perhaps FavoritesStore or similar is more appropriate.  You have some portions of code that are omitted that might suggest a more permanent data store being used in conjunction with this class, so perhaps this is really acting as a cache (FavoritesCache)?

So favorites seem to be their own entity in this context that should probably have their own class representation(s).

You might consider a structure like:

abstract class Favorite {
    const TYPE;
    public $id;
    public $ownerId;

    public function __construct($id, $ownerId)
    {
        $this->validatePositiveInteger($id);
        $this->validatePositiveInteger($ownerId);
        $this->id = $id;
        $this->ownerId = $ownerId;
    }

    protected function validatePositiveInteger($int)
    {
        if(!is_int($int) || $int < 1) {
            throw new InvalidArgumentException(
                'Positive integer value expected'
            );
        }
    }
}

class FavoriteHotel extends Favorite {
    const TYPE = 'Hotel';
}

class FavoriteRegion extends Favorite {
    const TYPE = 'Region';
}

// etc.

This also allows you to potentially tie different properties/behaviors to different types of favorites.  If you don't have the need to customize different favorites and are concerned over creating/managing a large number of favorite classes, you could alternately have just a single concrete class where upon instantiation you check against a valid list of favorite types.  In this case, perhaps consider extending this class from SplEnum so you can get enum-like behavior for your favorite types.

This would take the responsibility out of the store to enforce favorite types.

Also if you are treating this as a cache and your Favorite as a model for some record somewhere in a permanent store (i.e. database).  Then perhaps this class has methods on it to persist to permanent store or instantiate favorites objects from the permanent store.

So perhaps something like:

public function persist()
{
    // your logic to save to permanent store
}

public static function getInstanceById($id)
{
    // access your permanent store to get single record
    $class = 'Favorite' . $row->type;
    return new $class($row->id, $row->ownerId);
}

public static function getInstancesForOwnerId($ownerId)
{
    $instances = [];
    // access your permanent store to get records
    // for each record
    $class = 'Favorite' . $row->type;
    $instances[] = new $class($row->id, $row->ownerId);
    return $instances;
}

public static function deleteByOwnerId($ownerId)
{
    // your logic to delete records in permanent store
}

The last three methods might also be considered for separation into their own factory class (i.e. FavoriteFactory) if you need to expand on the means by which you instantiate favorite objects.

The takeaway here is that favorite class(es) represent the data model and own the means for how they are persisted into and recovered from the database.  Your simple in memory store/cache does not and should not need to know or understand this.

Your cache would just need to utilize this methods like:

// in save function after setting to cache
$favorite->persist();

// in get function when you have cache miss
$favorites = Favorite::getInstancesForOwnerId($ownerId);

// in reset function
$result = Favorite::deleteByOwnerId($ownerId);

I don't understand why you have buildResult method or have it exposed statically.  Do you really need a method to build an array? Why is this not just done in save().

i.e.

$this->data[$ownerId][] = ['id': $id, 'type': $type];

Also, if you consider making favorite into class as suggested, you can clean up your save() code like this:

public function save(Favorite $favorite) {
    $this->data[$favorite->ownerId][] = $favorite;
    // other stuff
    return true;
}

I don't see what API key is doing in this class, as it is not used anywhere.  I would also question why an API key would migrate as far down the call stack as where this store probably lives.  Typically, if you are exposing a service for GET'ing, POST'ing, PUT'ing, DELETE'ing data in a store, end user authentication via a mechanism such as an API key would be one of the first things done in validating the request. If the key is not valid, you should fail out the request with a 403 or whatever long before you get to the point of trying to set up data storage objects such as this.

Consider changing save method name to set and reset to unset as I think you would find these more typical method names for setting and unsetting items in a store or cache.

You are inconsistent in your approach to validating data passed to your public methods.  You do throw InvalidArgumentException in one case, but you do nothing to validate integer values for example.  Of course, that can get taken care of quite easily in some cases if you take the suggestion to put favorite into class that can be type-hinted against.

  /**
   * string
   */
  private $key;

Glad to see you using doc blocks, however these property blocks should probably be changed to @var string, @var array, etc.

You code indentation is inconsistent.

Consider making your code compliant against PSR-1 and PSR-2 since you are so close to being compliant already (i.e. you have generally good coding style).

That would require you to:

add namespace declaration
move opening brace for methods/functions onto line after declaration

The professional PHP development community has really gravitated towards the PSR standards in recent years, so probably best to be aware of them (and hopefully adopt them).  If you can configure you IDE and/or use style checker tools to enforce the coding standards, that is always a good approach.


Understanding OO JavaScript with a simple scenario Coming from a Java kind of OOP, I miss a lot of things in JavaScript. I am working on a Node.Js project, and I would like to use an OO approach.

Looking around and asking in StackOverflow I came up with this example scenario (jsFiddle).

function inheritPrototype(childObject, parentObject) {
    var copyOfParent = Object.create(parentObject.prototype)
    copyOfParent.constructor = childObject
    childObject.prototype = copyOfParent
}

//example
function Canvas (id) {
    this.id = id
    this.shapes = {} //instead of array?
    console.log("Canvas constructor called "+id)
}
Canvas.prototype = {
    constructor: Canvas
    , getId: function() {
        return this.id
    }
    , getShape: function(shapeId) {
        return this.shapes[shapeId]
    }
    , getShapes: function() {
        return this.shapes
    }
    , addShape: function (shape)  {
        this.shapes[shape.getId()] = shape
    }
    , removeShape: function (shapeId)  {
        var shape = this.shapes[shapeId]
        if (shape)
            delete this.shapes[shapeId]
        return shape
    }
}

function Shape(id) {
    this.id = id
    this.size = { width: 0, height: 0 }
    console.log("Shape constructor called "+id)
}
Shape.prototype = {
    constructor: Shape
    , getId: function() {
        return this.id
    }
    , getSize: function() {
        return this.size
    }
    , setSize: function (size)  {
        this.size = size
    }
}

//inheritance
function Square(id, otherSuff) {
    Shape.prototype.constructor.apply( this, arguments );
    this.stuff = otherSuff
    console.log("Square constructor called "+id)
}
inheritPrototype(Square, Shape)
Square.prototype.getSize = function() { //override
    return this.size.width
}

function ComplexShape(id) {
    Shape.prototype.constructor.apply( this, arguments );
    this.frame = null
    console.log("ComplexShape constructor called "+id)
}
inheritPrototype(ComplexShape, Shape)
ComplexShape.prototype.getFrame = function() {
    return this.frame
}
ComplexShape.prototype.setFrame = function(frame) {
    this.frame = frame
}

function Frame(id) {
    this.id = id
    this.length = 0
}
Frame.prototype = {
    constructor: Frame
    , getId: function() {
        return this.id
    }
    , getLength: function() {
        return this.length
    }
    , setLength: function (length)  {
        this.length = length
    }
}

/////run
var aCanvas = new Canvas("c1")
var anotherCanvas = new Canvas("c2")
console.log("aCanvas: "+ aCanvas.getId())

var aSquare = new Square("s1", {})
aSquare.setSize({ width: 100, height: 100})
console.log("square overridden size: "+aSquare.getSize())

var aComplexShape = new ComplexShape("supercomplex")
var aFrame = new Frame("f1")
aComplexShape.setFrame(aFrame)
console.log(aComplexShape.getFrame())

aCanvas.addShape(aSquare)
aCanvas.addShape(aComplexShape)
console.log("Shapes in aCanvas: "+Object.keys(aCanvas.getShapes()).length)

anotherCanvas.addShape(aCanvas.removeShape("supercomplex"))
console.log("Shapes in aCanvas: "+Object.keys(aCanvas.getShapes()).length)
console.log("Shapes in anotherCanvas: "+Object.keys(anotherCanvas.getShapes()).length)

console.log(aSquare instanceof Shape)
console.log(aComplexShape instanceof Shape)

Is this approach correct (best practices)? Should I do something differently?

The idea would be then to create a js file for each class and use module.exports.. in Node.Js I will also use Mongoose to store the objects into a database, I guess I will need a way to translate my objects into Mongoose Models and back..

EDIT: how would one implement private methods used internally?

Pretty good code.

From an OOP perspective, this is exactly right. From a formatting perspective, this is almost flawless. I hardly have anything to say about this code, but I see very little that I would change.

The only issue that I see is an irregular formatting choice. These commas:

Shape.prototype = {
    constructor: Shape
    , getId: function() {
        return this.id
    }
    , getSize: function() {
        return this.size
    }
    , setSize: function (size)  {
        this.size = size
    }
}

should be like this:

Shape.prototype = {
    constructor: Shape,
    getId: function () {
         return this.id
    },
    getSize: function () {
         return this.size
    },
    setSize: function (size)  {
        this.size = size
    }
};

Oh and you need to read this world-changing answer about OO practices in JS.

Be sure to press B while reading that, because if not, you'll evolve into a legendary OOP developer.


Doubly linked list of strings This code works just fine as I get the correct output. However I want to make my code more simple. I want to avoid using the setPrev(Node n) method and would rather set the reference to the previous node in the Node(String s) constructor directly somehow. I mainly want to know if this is the 'right' way to implement a Doubly linked list though.

public class DoublyLinkedList {
    private Node current;
    private Node first;
    private Node traverse;
    private int count = 0;
    public DoublyLinkedList(String s)
    {
        first = new Node(s);
        current = first;
    }
    public void add(String s){
        current.next = new Node(s);
        current.next.setPrev(current);
        current = current.next;
    }
    public void traverseForward(){
        count++;
        if(count==1)
            traverse = first;
        else
            traverse = traverse.next;
        System.out.println(traverse.toString());
    }
    public void traverseBackward(){
        count--;
        if(count < 1)
            traverse = null;
        else if(count == 1)
            traverse = first;
        else
            traverse = traverse.prev;
        System.out.println(traverse.toString());
    }
    private class Node{
        private Node next;
        private Node prev;
        private String name;

        public Node(String s)
        {
            this.name = s;
            this.next = null;
        }
        public void setPrev(Node n){
            this.prev = n;
        }
        public String toString(){
            return this.name;
        }
    }
}

There are a few things I see with your code that I would change:

Node API

First, I would make the members of class Node public. As it stands now, you only have one setter function to the prev member and that's it. This kind of defeats the purpose of making the entire class private to the DoublyLinkedList. Note that setting these members to be public doesn't break good encapsulation practice since the only class able to access and modify those members is the DoublyLinkedList. Moreover, why doesn't your constructor initialize prev? I would also add one or two more constructors that would allow me to construct a Node and assign the values of the next and prev references. This is how I'd revise class Node:

private class Node{
    public Node next;
    public Node prev;
    public String name;

    public Node(String s)
    {
        name = s;
        next = null;
        prev = null;
    }
    public Node(String s, Node n, Node p)
    {
        name = s;
        next = n;
        prev = p;
    }
    public String toString(){
        return this.name;
    }
}

LinkedList API

Instead of providing traverseBackward() and traverseForward(), you may want to have your class implement Iterable and/or Iterator and provide an iterator interface to the user. Also, I would provide a size() or length() member function to return the size of the list. Otherwise, why bother having or keeping track of its size? I'd also throw in an insert(), is_empty(), clear(), and remove() function to the API. Moreover, this is very unnecessary:

private Node current;
private Node first;
private Node traverse;

All you need to keep track of in the list is the head of the list (and maybe the tail if you want O(1) insertion at the end of the list instead of O(n):

private Node head, tail;


Mouse input handler which exposes read-only mouse state data I have a MouseInputHandler class that registers itself to an InputManager. When the InputManager class receives events they are forwarded to MouseInputHandler which changes its state to reflect the state of the mouse. A mouse as an input device provides a large number of different properties that should be easy to read, but impossible to write to.

The problem is that C++ fails to provide a keyword otherwise similar to const but that grants full write access to functions in the same scope.

class MouseInputHandler : public InputHandler
{
public:
    MouseInputHandler(ResourceContext &context)
        : btnLeft(btnLeft_), btnMiddle(btnMiddle_), btnRight(btnRight_),
          btnX1(btnX1_), btnX2(btnX2_), absX(absX_), absY(absY_), relX(relX_),
          relY(relY_), scrollX(scrollX_), scrollY(scrollY_) {}
    void handleEvent(SDL_Event &mouseEvent);
    void refresh();

    const Signal &btnLeft, &btnMiddle, &btnRight, &btnX1, &btnX2;
    const int32_t &absX, &absY, &relX, &relY, &scrollX, &scrollY;
protected:
    Signal btnLeft_, btnMiddle_, btnRight_, btnX1_, btnX2_;
    int32_t absX_, absY_, relX_, relY_, scrollX_, scrollY_;
};

Options I have identified at this point:

Declaring the members private or protected and providing public const references to the non-public members that only allow read access (as in the example above). However with 11 members, the class quicly becomes a mess, with 11 const references (that also require 11 reference initializations in the constructor's initializer list).
Adding getter methods for every member variable. More cumbersome than the const references, but with the (marginal) added benefit that the members are now accessible as functions.
Placing the buttons and integers in (private or protected) arrays and adding an enum that gives indexes to the array elements meaningful names. To access the members, two (one getter for buttons, other for ints) getter functions that take the index and return a const reference are added to the class. This seems to me quite complex and error prone in comparison to the other solutions.
Just not caring about const correctness at all and making the member variables fully accessible.
Refractoring the code. I don't see how one could refractor a mouse, or simplify the interface, but any suggestions are welcome.

You could represent the mouse state using a struct:

struct MouseSettings
{
    Signal btnLeft_, btnMiddle_, btnRight_, btnX1_, btnX2_;
    int32_t absX_, absY_, relX_, relY_, scrollX_, scrollY_;
}

class MouseInputHandler : public InputHandler
{
public:
    MouseInputHandler(ResourceContext &context)
        : mouseSettings(mouseSettings_)
    {}
    void handleEvent(SDL_Event &mouseEvent);
    void refresh();

    const MouseSettings& mouseSettings;

protected:
    MouseSettings mouseSettings_;
};

Or a property-like get-accessor method like:

const MouseSettings& getMouseSettings() { return mouseSettings_; }


Toy Vector Library - Magnitude & Unit Vector Functions I'm writing a toy vector library in C to learn a bit about the language. The excerpt below shows my vector structure and two functions:

vec2_magnitude calculates the magnitude (length) of a vector
vec2_unit uses the magnitude to calculate the unit vector

#include <math.h>

// A fixed-length vector w/ 2 elements.
typedef struct
{
        double a;
        double b;

} vec2_t;

// Compute the magnitude of a vec2_t.
double vec2_magnitude(const vec2_t *vec)
{
        double a2 = vec->a * vec->a;
        double b2 = vec->b * vec->b;
        return sqrt(a2 + b2);
}

// Compute the unit vector of a vec2_t.
void vec2_unit(const vec2_t *vec, vec2_t *unit)
{
        double mag = vec2_magnitude(vec);
        unit->a = (vec->a / mag);
        unit->b = (vec->b / mag);
}

Currently, vec2_unit takes two vector arguments. It performs the calculation based on the first argument, then stores its result in the second argument. I wrote the function this way so the original vector wouldn't be affected (in case it is still needed).

That being said, I'd like a way to calculate the unit vector in-place, since you may not always need the old vector (in such a situation, an in-place calculation is more convenient). I was originally going to write two functions, but noticed a clever way to achieve both functionalities using only the function shown above.

To operate non-destructively, vec2_unit can be called like this:

vec2_t orig_vector = {1, 1};
vec2_t unit_vector;
vec2_unit(orig_vector, unit_vector);

To operate in-place, vec2_unit can be called like this:

vec2_t orig_vector = {1, 1};
vec2_unit(orig_vector, orig_vector);

I like this solution, and it seems to work. I also understand that C is a language where many things aren't "safe", and just because you can do something doesn't mean you should. Are there any reasons something like this shouldn't be done? Are there other examples of common C functions that work in a similar way? Is there a better way to accomplish what I'm trying to do?

I appreciate all help. Feel free to comment on anything else you see as well. Thanks!

It is custom/industry de facto standard to place the parameter to be changed first, as done in memcpy, strcpy etc (this tradition goes all the way back to assembler).

So you should do void vec2_unit (vec2_t* dst, const vec2_t* src);.
Regarding in-place or not, it is mostly a matter of style in this case.

Functions/APIs working on "immutable" objects (objects not modified by the function) are often considered better style when that option is available, since that minimizes the chance of caller-side bugs.

The discussion about in-place vs immutable mostly makes sense for larger data types, where taking a copy of the data object is regarded as costly. A struct with 2 double isn't really that heavy to copy(note 1), so you could design this API with an immutable object interface. You could even get away with passing the structs to/from the functions by value, which is otherwise normally frowned upon.

Note: if these functions will reside in the same translation unit as the caller code (which doesn't seem likely here?), the whole discussion about performance is pointless since they will be inlined anyway in that case.
If you go for the immutable version, it could be written as

void vec2_unit (vec2_t* restrict dst, const vec2_t* restrict src);

This tells the compiler and the caller both that these two objects shall not be the same one. That will in turn increase performance, but it will not allow you to pass the same object as both source and destination.

(note 1) We can assume that systems using double are able to copy it in a few instructions. Systems where copying a double would be lots of work, such as small microcontroller systems, shouldn't be using double (or float) in the first place, since they lack a FPU. I would safely assume that any program using floating point is meant to run on a Cortex M3 or bigger.


Finding the 20th percentile pixel of an image using a histogram instead of std::sort I have a 2-dimensional matrix (an image) in which I need to find the 20th percentile value.  My first attempt was to sort the values and then index using std::floor(0.2*(srcSor.size())/100).

My code was originally straightforward like this:

#include <opencv2/highgui.hpp>
#include <opencv2/imgproc.hpp>

#include <iostream>

int main(int argc, char* argv[])
{
   //Read the image .jpeg
   cv::Mat src=cv::imread(argv[1], CV_8UC1);
   std::vector<double> srcSor = src.reshape(0, 1);
   std::sort (srcSor.begin(), srcSor.end(), std::greater<double>());
   float A = srcSor[std::floor(0.2*(srcSor.size())/100)];
   //Then later I use A value..
   .
   .

   return 0;
}

I found that std::sort took a long time to sort a vector of 1.3m unsigned values.
So now I'm using an histogram to do the same thing; this takes less time to find that value of that certain index. The idea comes from the fact that the values of the matrix are unsigned int in the range 0-255:

#include <opencv2/highgui.hpp>
#include <opencv2/imgproc.hpp>

#include <iostream>

int main(int argc, char* argv[])
{
   //Read the image .jpeg
   cv::Mat src=cv::imread(argv[1], CV_8UC1);

   std::vector<unsigned> histo(256,0.0);
   for(unsigned j=0;j<src.rows;j++)
      for(unsigned i=0;i<src.cols;i++)
          histo[(int)src.at<uchar>(j,i)]++;

   std::vector<unsigned> sumHH(256,0.0);
   sumHH[255]=histo[255];
   for(unsigned i=254;i>=1;i--)
        sumHH[i]=sumHH[i+1]+histo[i];

   int A=0;
   int indx=std::floor(0.2*(src.rows*src.cols)/100);
   for(unsigned i=1;i<255;i++)
     if(indx<sumHH[255-i])
     {
        A=255-i;
        break;
     }
   //Then later I use A value..
   .
   .

   return 0;
}

But I think that my code is not really readable, can anyone suggest a better way to write my code and/or better optimized?

Well, index=std::floor(0.2*(srcSor.size())/100 is equivalent to the much simpler index = srcSor.size() / 500, ignoring some inaccuracy in representing 0.2.
Lose the fixed-size std::vectors. You don't depend on any of the benefits of using dynamic memory, so a simple array is sufficient and far more efficient.
Next, you really don't need the cumulative counts in sumHH at all. Just test whether you have reached the target immediately.
Don't cast unless you have to. Casting means overriding the type-system, and is thus error-prone.
Don't use obscure abbreviations.

Resulting code:

unsigned histogram[256] = {};
for (auto j = src.rows; j--;)
    for (auto i = src.cols; i--;)
        ++histogram[src.at<uchar>(j,i)];
int A = 255;
for (auto index = src.rows * src.cols / 500; index > histogram[A]; --A)
    index -= histogram[A];


Random select in two MySQL tables I wrote a simple AJAX request that performs a random select in two MySQL tables. My current script involves connecting to the database and creating a new PDO every time the #generate button is clicked.

Can it be considered a bad practice? Could it lead to performance issues? If not, how could it still be optimized?

PHP (generator.php)

$host = "localhost";
$dbname = "generator";
$user = "root";
$password = "";
$utf8 = array(PDO::MYSQL_ATTR_INIT_COMMAND => "SET NAMES utf8");

try {
    $database = new PDO("mysql:host=$host; dbname=$dbname", $user, $password, $utf8);
    $database->setAttribute( PDO::ATTR_ERRMODE, PDO::ERRMODE_EXCEPTION );
}
catch(PDOException $e) {
    file_put_contents('PDOErrors.txt', $e->getMessage(), FILE_APPEND);
    die();
}

$query = "SELECT * FROM (
    (SELECT * FROM homemade WHERE validation = 1 ORDER BY RAND() LIMIT 0,1)
    UNION ALL
    (SELECT * FROM user_generated WHERE validation = 1 ORDER BY RAND() LIMIT 0,1)
)
AS foo
ORDER BY RAND()
LIMIT 0,1";

$statement = $database->prepare($query);
$statement->execute();
$content = $statement->fetch();

$data = array();

if($query) {
    $data["pickedName"] = $content["name"];
    $data["pickedBaseline"] = $content["baseline"];
}

echo json_encode($data);

JS

var $name = $("#content h2");
var $baseline = $("#content h3")

$("#generate").click(function() {
    $.ajax({
        type: "POST",
        url: "generator.php",
        dataType: "json"
    }).done(function(data) {
        $name.text(data.pickedName);
        $baseline.text(data.pickedBaseline);
    }).fail(function(data) {
        $name.text("Something went wrong !");
        $baseline.text("Please try again.");
    });
});

If you are careful about it, instead of creating a new PDO instance on every invocation of your script, you can use a persistent connection. Make sure you resolve all transactions through a commit() or rollBack().

To make your connection persistent, just include PDO::ATTR_PERSISTENT => true in your options array when constructing your PDO object.

And now, for a review of your code:

Storing database keys directly in your PHP file is considered bad practice. Generally it's better to put it somewhere outside your web root and require it in, or on Apache, to set appropriate php_values in your .htaccess. Alternatively, and again on Apache, use SetEnv in your httpd.conf.
$utf8 is a misleading name. It actually stores your PDO options. If you extend your options to include PDO::ATTR_PERSISTENT, then the variable name will no longer make sense.
Your query is inefficient. ORDER BY RAND() is slow enough as it is, and you're using it twice! (The third time is over two rows, so that's not a big issue.) Here's an excellent article on quickly selecting random rows in MySQL.
You're doing a SELECT *. It's better to explicitly mention what columns you're using.
If my assumptions are correct, homemade and user_generated have the same columns. I'm not sure this is a good idea; I think it would be better to put them in the same column. Note: as it is, your code will have 50% of picking a user generated pair of values, and 50% of picking a "homemade" pair, regardless of the ratio of user generated to homemade. You'll need to account for that if that's the desired behaviour.
Minor stylistic points: be consistent in spacing around parentheses. Also, catch is typically placed on the same line as the precending }.


Hangman game in Python While I'm not new to Python, this is my first 'bigger' Python program I created, also my first game in Python.

import random

DEBUG = False; # If set to true, displays chosen word before each game. For debugging and cheating only :)

DICT_FILE = "english.txt";
GUESSES   = 10;

def prompt(s):
    while True:
        ans = raw_input(s + " (Y/n) ").lower();
        if ans in ["y", "n"]:
            return ans == "y";

def loadWords():
    with open(DICT_FILE, "r") as f:
        return f.read().strip().split("\n");
                       # Strip in case of newline at end of file

def play(word):
    word = word.lower();
    if DEBUG:
        print "[DEBUG] word is %r" % (word);
    guesses = GUESSES;
    toGuess = set(l for l in word.upper() if l.isalpha()); # Handles apostrophes, etc.
    letters = set();
    while guesses > 0:
        if len(toGuess) == 0:
            print "You win with %d guess%s left!" % (guesses, "" if guesses == 1 else "es");
            print "The word is %r" % (word);
            return True;
        print "=" * 78;
        print "You have %d guess%s left." % (guesses, "" if guesses == 1 else "es");
        print "Word:    " + " ".join("_" if c in toGuess else c for c in word.upper());
        print "Letters: " + " ".join(sorted(letters));
        while True:
            l = raw_input("Choose a letter: ").upper();
            if l.isalpha():
                if l not in letters:
                    letters.add(l);
                    if l in toGuess:
                        toGuess.remove(l);
                        print "Correct!";
                    else:
                        guesses -= 1;
                        print "Sorry, not in the word.";
                    break;
                else:
                    print "You already chose that letter!";
            else:
                print "That's not a letter!";
    print "=" * 78;
    print "Sorry, you lose.";
    print "The word was: %r" % (word);
    return False;

if __name__ == "__main__":
    words = loadWords();
    random.shuffle(words);
    while True:
        play(words.pop()); # Remove a random word so it's not chosen again
        if not prompt("Would you like to play again?"):
            break;

I'd like to know what you think and what I should change.
By the way, english.txt placed in the same folder as the game script contains phrases/words separated by newlines that should appear in the hangman game.

This is Python and you do not need the Semi-Colons (;)

Your code looks absolutely non-pythonic like that, it looks like it is trying to morph into Java or something.

You should also separate some more code into methods/functions like this one here

print "=" * 78;

I believe that this is to separate the game from the ending notifications.

What gave this away was 78 I have no clue what that is, and if I have different settings on my console/IDE then this won't look right anyway.  78 is a magic number and should be dealt with so that I know what it means


Invoking subscribers of an action in parallel c# I have a questions as to the proper way to structure the code below.

This snippet of code is used within a socket class which is intended to be a high performance socket. Many data handlers subscribe to this sockets OnData event to handle different types of data. Ideally, I'm trying to kick the data handlers off in a task and in parallel where the HandleData method is invoked once the socket has received data.

1) Should I make this method async and await the task?

2) Should I capture a copy of the delegates inside the task?

3) Is there a better way to accomplish this?

    private void HandleData(string message)
    {
        var data = JObject.Parse(message);

        var delegates = OnData?.GetInvocationList();

        if (delegates != null)
        {
            Task.Run(() =>
            {
                Parallel.ForEach(delegates, d =>
                {
                    try
                    {
                        d.DynamicInvoke(data);
                    }
                    catch (Exception)
                    {
                        /* Ignore */
                    }
                });
            });
        }
    }

I think that unless benchmarking proves otherwise, you should simply OnData?.BeginInvoke() and just let the thread pool handle it for you.


Add and enable/disable Windows Firewall rules with Python I have this following module using for adding and enabling/disabling Windows Firewall rules using Python.

I currently use subprocess.call to execute the netsh command inside Python. I'm wondering if there is any better method to do this? Executing the cmd command inside Python seems to be impractical to me.

import subprocess, ctypes, os, sys
from subprocess import Popen, DEVNULL

def chkAdmin():
    """ Force to start application with admin rights """
    try:
        isAdmin = ctypes.windll.shell32.IsUserAnAdmin()
    except AttributeError:
        isAdmin = False
    if not isAdmin:
        ctypes.windll.shell32.ShellExecuteW(None, "runas", sys.executable, __file__, None, 1)

def addRule(rule_name, file_path):
    """ Add rule to Windows Firewall """
    subprocess.call("netsh advfirewall firewall add rule name="+ rule_name +" dir=out action=block enable=no program=" + file_path, shell=True, stdout=DEVNULL, stderr=DEVNULL)
    print("Rule", rule_name, "for", file_path, "added")

def modifyRule(rule_name, state):
    """ Enable/Disable specific rule, 0 = Disable / 1 = Enable """
    if state:
        subprocess.call("netsh advfirewall firewall set rule name="+ rule_name +" new enable=yes", shell=True, stdout=DEVNULL, stderr=DEVNULL)
        print("Rule", rule_name, "Enabled")
    else:
        subprocess.call("netsh advfirewall firewall set rule name="+ rule_name +" new enable=no", shell=True, stdout=DEVNULL, stderr=DEVNULL)
        print("Rule", rule_name, "Disabled")

chkAdmin()
addRule("RULE_NAME", "PATH_TO_FILE")
modifyRule("RULE_NAME", 1)

I agree with @Ludisposed answer, but you have a few subprocess gotchas:

You don't need to spawn a shell in order to run the command, simply build your command as a list of arguments and it will be fine. This is especially important if your rules names may contains spaces as the command would be treated entirelly differently in your implementation;
Replace the old subprocess.call by subprocess.run;
You may be interested to run subprocess.run by specifying check=True in order to generate an exception and be alerted if something does not go according to plan.

Applying these changes to e.g. modify_rule can lead to:

def modify_rule(rule_name, enabled=True):
    """Enable or Disable a specific rule"""
    subprocess.run(
        [
            'netsh', 'advfirewall', 'firewall',
            'set', 'rule', f'name={rule_name}',
            'new', f'enable={"yes" if enabled else "no"}',
        ],
        check=True,
        stdout=DEVNULL,
        stderr=DEVNULL
    )

Also note that I removed the print call from the function as it impairs reusability. If the caller want this kind of messages, it should be responsible for printing them, not this function.


Last three values sfdc trigger functionalty Please review the following salesforce trigger code

trigger BrandTrigger on Brand__c (before update) {
    for(Brand__c brand: Trigger.new){
        Brand__c oldBrand = Trigger.oldMap.get(brand.Id);
        if (oldBrand.Name != brand.Name) {

            if(String.isBlank(oldBrand.Old_Name__c)){
                brand.Old_Name__c = oldBrand.Name;
            }else{
                brand.Old_Name__c = split(oldBrand.Name + ', ' + brand.Old_Name__c );
            }
        }
    }

    private String split(String s) {
        Integer index = 0;
        for (Integer i = 0; i < 3; i++)
            index = s.indexOf(', ', index + 1);

        if(index > 2)
            return s.substring(0, index);
        else
            return s;
    }
}

Trigger sets at least three values in the "Old Value" field. These are the historical values of Brand_c.Name

You should try to keep the logic out of the trigger itself and try to put it into a handler class. This goes along with the one trigger per object pattern. What would happen if you needed to add another trigger on the same object that performed logic after update or before insert?
It looks like you might be replicating standard functionality with your split function. Make sure the standard split method not will meet your needs.


Add three integers and report the sign of the sum I am in my first high school programming course; the assignment is:

  Write a program that asks the user to input three integers and outputs
  a message indicating their sum and whether it is positive, negative,
  or zero. You should create 2 classes to run this program. One class
  contains all of the procedures for your program and the other has all
  of the procedure calls for your program.

I have done exactly what the assignment has asked for me to do; I was wondering if there was anything that I should change to make my code more simple and or more elegant or point out anything that might lead up to errors, since I'm in my first programming course. I really want to start programming with simple elegant code and not messy code, because I feel like it starts good habits for me later on.

import java.util.Scanner;

class ConorsList {
    int total;

    public void add(int number) {
        this.total = this.total + number ;
    }

    public void status(){
        if(this.total<0) {
                System.out.println("The sum is negative.");
        } else if (this.total>0) {
                System.out.println("The sum is positive");
        } else {
               System.out.println("The sum is equal to zero");
        }
    }

    public void status(int total){
        if(total<0) {
                System.out.println("The number is negative.");
        } else if (total>0) {
                System.out.println("The number is positive.");
        } else {
               System.out.println("The number is equal to zero.");
        }
    }
}

public class Class1_2 {

   public static void main(String args[]) {
       ConorsList list = new ConorsList();
       Scanner fromKeyboard = new Scanner(System.in);

       int amount = 3;

       for (int n=0; n<amount; n++) {
           int x =fromKeyboard.nextInt();
            list.add(x);
            list.status();
            list.status(x);
       }

   }

}
```

Use shorthand for addition, multiplication, division etc.

E.G instead of this.total = this.total + number;

do: this.total += number;

You have a lot of code repetition with these two methods. Anytime you copy & paste, you should be asking yourself if you can make the code shorter.

With your overloaded method, it's easy. Just change your no-arg constructor to call the other:

public void status(){
    status(this.total);
}

You may also want to change the name to printStatus. You can further reduce the code, since all of the if statements print a lot of the same characters

E.G:

public void status(total){
    String status = "The sum is ";

    if(total<0) {
            status += "negative.";
    } else if (total>0) {
            status += "positive";
    } else {
           status += "equal to zero.";
    }

    System.out.println(status);
}


What parts of my Python code are adopting a poor style and why? Here I have some simple code that worries me, look at the second to last line of code, I have to return spam in order to change spam in the global space. Also is it bad to be writing code in this space and I should create a function or class to call from that space which contains most my code?

I found out very recently that I never even understood the differences between Python's identifiers compared to C or C++'s variables. This helps put into perspective why I always have so many errors in Python.

I do not want to continue writing code and get into a bad habit and this is why I want your opinion.

I also have a tendency to do things like spam = fun() because I do not know how to correctly make Python call a function which changes a variable in the scope of where the function was called. Instead I simply return things from the functions never learning how to correctly write code, I want to change that as soon as possible.

import random
MAX = 20

def randomize_list(the_list, x=MAX):
    for i in range(len(the_list)):
        the_list[i] = random.randint(1, x)

def lengthen_list(the_list, desired_length, x=MAX):
    if len(the_list) >= desired_length:
        print "erroneous call to lengthen_list(), desired_length is too small"
        return the_list
    while(len(the_list) < desired_length):
        the_list.append(random.randint(1, x))
        # organize the_set
        the_list = set(the_list)
        the_list = list(the_list)
    return the_list

spam = list(range(10))
randomize_list(spam, MAX)

print "the original list:"
print spam, '\n'

spam = set(spam)
spam = list(spam)

print "the set spam:"
print spam

print "the set spam with length 10"
spam = lengthen_list(spam, 10, MAX)
print spam

Please help me adopt a style which correctly fixes, in particular, the second to last line of code so that I do not need the function to do return things in order to work, if this is even possible.

I also have a tendency to do things like spam = fun() because I do not
  know how to correctly make Python call a function which changes a
  variable in the scope of where the function was called.

You can't. You cannot replace a variable in an outer scope. So in your case, you cannot replace the list with a new list, although you can modify the list. To make it clear

variable = expression

replaces a variable, whereas pretty much anything else:

variable[:] = expression
variable.append(expression)
del variable[:]

Modifies the object. The bottom three will take effect across all references to the object. Whereas the one before replaces the object with another one and won't affect any other references.

Stylistically, it somestimes makes sense to have a function modify a list, and other times it makes sense for a function to return a whole new list. What makes sense depends on the situation. The one rule I'd pretty much always try to follow is to not do both.

def randomize_list(the_list, x=MAX):
    for i in range(len(the_list)):
        the_list[i] = random.randint(1, x)

This isn't a good place to use a modified list. The incoming list is ignored except for the length. It'd be more useful as a function that returns a a new list given a size something like:

def random_list(length, maximum = MAX):
    random_list = []
    for _ in range(length):
        random_list.append( random.randint(1, maximum) )
    return random_list

def lengthen_list(the_list, desired_length, x=MAX):
    if len(the_list) >= desired_length:
        print "erroneous call to lengthen_list(), desired_length is too small"

Don't report errors by print, raise an exception

 raise Exception("erroneous call...")

        return the_list
    while(len(the_list) < desired_length):
        the_list.append(random.randint(1, x))
        # organize the_set
        the_list = set(the_list)
        the_list = list(the_list)

That's not an efficient way to do that. You constantly convert back between a list and set. Instead do something like

 new_value = random.randint(1, maximum)
 if new_value not in the_list:
      the_list.append(new_value)

That way you avoid the jumping back and forth, and it also modifies the original object rather then creating a new list.

    return the_list

This particular function could go either way in terms of modifying or returning the list. Its a judgement call which way to go.

  Also is it bad to be writing code in this space and I should create a
  function or class to call from that space which contains most my code?

Yes, you should really put everything in a function and not at the main level for all but the most trivial scripts.


Make cd command work with spaces Some archives contain folders with spaces in their name.
Sometimes while I'm browsing these folders, I want to open a terminal in current position, so I copy the path to paste it after a cd command.
Of course when there are spaces in the path, cd won't work unless all spaces are escaped.

Since I find it annoying not to be able to paste paths after cd, I just added this function to my .bash_aliases:

function cd {
  if [ -z "$*" ]; then
    command cd
  else
    command cd "$*"
  fi
}

So mkdir "an awful dir name" && cd an awful dir name now works as expected (what else should cd an awful dir name do?).

Since .bash_aliases shouldn't affect non-interactive shells, other scripts should still work with builtin cd.

Now the question is: can this trick cause problems on my shell? Or am I freaking out for no reason?

Throw away your current code. Instead write this:

for dir in *; do
  (cd "./$dir" && echo "In $(exec pwd) now.")
done

As a general rule, always put variables into quotes.

Rationale: Your current code can handle a few cases properly, but some other special characters will still be interpreted specially by the shell and lead to unexpected behavior. Therefore the proper way to work around this problem is to write cd ' into the terminal, then to paste the path and then to finish the command by typing '. This will work for all characters except single quotes.

Alternatively, you could invent a command cdclip that changes to the path in the clipboard. Cygwin has /dev/clipboard, and on native Linux there is probably some similar way to read the clipboard contents.


Function that shows markers all at once or with a delay I have some markers on an image. At some point all these markers are inactive (i.e. a class name is added to the element, which hides them) and need to be shown again. This is done by the method showAll() of a class (it's not a loose function).

This function works like this:

if no delay is given as a parameter, all markers are shown right away
if delay is "true", the markers are shown one after another

This is my implementation:

showAll(delay) {
    for (let i = 0; i < this.elements.length; ++i) {
        const element = this.elements[i];

        if (delay) {
            element.classList.remove('inactive');
        } else {
            setTimeout(e => {
                element.classList.remove('inactive');
            }, this.delay * i);
        }
    }
}

A few words to understand all variables:

this.elements is a list of cached DOM elements
this.delay is simply the delay in ms

I'm concerned about the branching and the repetition of element.classList.remove('inactive');. However I'm not a fan of this solution:

setTimeout(e => {
    element.classList.remove('inactive');
}, delay ? this.delay * i : 0);

It seems unecassary to call setTimout() anyway.

Is there any way to improve this code?

Make a helper function
One way to simplify this is to use a function to abstract removing the class name from the element. For example, create one like removeInactiveClassToElement:
removeInactiveClassToElement = (el) =>    el.classList.remove('inactive');

Then just call that function when necessary. For the case when calling in setTimeout(), use Function.bind() to make a partially-applied function, passing null as the thisArg argument, and then element as the first argument.
if (delay) {
    setTimeout(removeInactiveClassToElement.bind(null, element), delay * i);
}
else {
    removeInactiveClassToElement(element)
}

As you can see above, I switched the logic around so that the delay is added when delay has a value, and used that instead of this.delay, though if you wanted to set that property on the instance you could do that...
Functional approach
One could also use Array.forEach() instead of using the for statement. While it could slow performance (due to an extra function call on each iteration), it could cut the first couple lines out (e.g. assigning const element = this.elements[i];).
this.elements.forEach((marker, i) => {
    if (delay) {
        setTimeout(removeInactiveClassToElement.bind(null, marker), delay * i);
    }
    else {
        removeInactiveClassToElement(marker)
    }
});

Expand the snippet below for a demonstration, where clicking the image will add marker dots.

//helper functions
addInactiveClassToElement = (el) => el.classList.add('inactive');
removeInactiveClassToElement = (el) =>    el.classList.remove('inactive');
class MarkerList {
  constructor() {
    this.elements = [];
  }
  add(x, y) {
    const marker = document.createElement('div');
    marker.classList.add('marker');
    marker.setAttribute('style', 'left: ' + x + 'px; top: ' + y + 'px;');
    document.body.append(marker);
    this.elements.push(marker);
  }
  hideAll() {
    this.elements.forEach(addInactiveClassToElement);
  }
  showAll(delay) {
    this.elements.forEach((marker, i) => {
      if (delay) {
        setTimeout(removeInactiveClassToElement.bind(null, marker), delay * i);
      }
      else {
        removeInactiveClassToElement(marker)
      }
    });
  }
}
document.addEventListener('DOMContentLoaded', function() {
  var markers = new MarkerList();
  this.addEventListener('click', function(clickEvent) {
    if (clickEvent.target.id == 'hide') {
      markers.hideAll();
    } else if (clickEvent.target.id == 'show') {
      markers.showAll(delay.value);
    } else if (clickEvent.target.id == 'image') {
      markers.add(clickEvent.clientX, clickEvent.clientY);
    }
  });
});
.marker {
  background-color: #f00;
  width: 2px;
  height: 2px;
  position: absolute;
}
.inactive {
  display: none;
}

<button id="hide">
  Hide all
</button>
<button id="show">
  Show all
</button>

 Delay:
 <input id="delay" type="number" value="2000" />