Plot on a 2D grid a spherically symmetric function I am trying to compute a spherically symmetric function on a 2D grid. The simple solution that I came up with is the following:

Grid_side = 10
N_pix = 100

function = lambda r : 1.0 / r**2.0

grid_x = np.linspace(-Grid_side, Grid_side, N_pix)
grid_y = np.linspace(-Grid_side, Grid_side, N_pix)

SphericallySymmetric_function_GRID = np.zeros((N_pix, N_pix))

for i in range(N_pix):

        for j in range(N_pix):

            SphericallySymmetric_function_GRID[i,j] = function(np.sqrt(grid_x[i]**2.0 + grid_y[j]**2.0))

I was wondering if there is a better way to do the following, taking advantage of Numpy.

No need to iterate if the function works with arrays, and you use broadcasting to populate a grid:

def fn(X,Y):
    r = np.sqrt(X**2+Y**2)
    return 1/r**2
fn(grid_x[:,None], grid_y[None,:])

np.meshgrid and np.mgrid (or np.ogrid) can also be used to form the X,Y grid.

fn(*np.meshgrid(grid_x, grid_y))
fn(*np.ogrid[-Grid_side:Grid_side:N_pix*1j, -Grid_side:Grid_side:N_pix*1j])

Look up their docs, and practice.  Practice with simple things like grid_x[:,None]+grid_y as well.


Is it better practice to have void method throw an exception or to have the method return a boolean? This falls straight into the same category with the recent "Is it better practice to return if false or execute an if statement if true?" question.

Often, while writing code, I find myself presented with two options. Either I can do this:

public void myMethod() throws Exception {
    // Some code
    validate(some_object_or_condition);
    // Some code
}

private void validate(Object arg) throws Exception {
    // Code that validates the argument and throws an exception if it's invalid
}

Or this:

public void myMethod() throws Exception {
    // Some code
    if( !isValid(some_object_or_condition) ) throw new Exception();
    // Some code
}

private boolean isValid(Object arg) {
    // Code that returns a boolean based on whether or not the argument is valid
}

Is either one preferable over the other? When I was still just learning Java the latter felt more "object oriented" and somehow more natural. However, with experience the former has started to feel better because then the code looks cleaner to me, but I'm afraid people might find it less readable.

Edit/Add: This time I ran into this problem when implementing a game (just for the fun of it) where the user would place words into a grid. The playWord method may fail because of the word not actually being a real word (yielding an InvalidWordException) or because the user is trying to place a valid word into an illegal position (yielding a WordPlacementException). If the method does not fail, it should return the score of the word as an integer. So in my case the playWord method first calls the method that validates the word and later the method that validates the location of the word, and these provoked this question here.

There's quite a few questions about this on stackoverflow but I think my general rule tends to be leave exceptions for exceptional circumstances and booleans for where failure occurances is an accepted/known possible behaviour.

So I personally probably prefer your option 2 (interested to see what others think), where if anything the exception is thrown a bit higher up the chain.  That way I could re-use the isValid logic elsewhere and deal with the failure without needing to wrap it in an exception handler.

Of course, I think this is probably a case by case basis and depends on the kind of logic being checked.

I'm sure I remember a really good Q&A on this somewhere but can't quite find it.  However found a couple of discussions:

Throw exception or return boolean - SO

Throwing exceptions - SO

EDIT:  To further comment after the edits, perhaps you could also consider the option of returning status codes (enums) instead of the exceptions if there are a number of options.  This has the benefit of being more efficient however as some have mentioned on SO could expose unecessary workings if this is a API or assembly.  Some links I found on SO about this with some good discussions were:

Exception or error code enum
to use error codes or not

And there were more.  I see this coming down to a few options.

Throw exception
Return true/false
Return a status/error code enum

All probably have their place.  Exceptions do occur the most overhead so if performance is an issue or the routine is called in many places perhaps this is not the best solution.  I also saw mention that it was not necessarily a good idea to use exceptions to control the flow of application logic which it would seem to be doing in your case?

Basically my thoughts are to use return true/false, use a status error code, and then exceptions in that order.

An alternative suggestion might be something along the lines of (whether it's a decent alternative or not hopefully someone might comment on ???):

public int playWord(string w) {
    Word word = new Word(word);
    if (word.isValid()) {
       return word.getScore();
    } else {
       throw new InvalidDataException(word.GetErrorMessage());
    }
}

class Word {
   private String word;
   private String errorMessage = null;

   public Word(string w) {
       word = w;
   }

   public boolean IsValid() {
       // TODO: perform various checks calling private methods as required and if fail store against errorMessage
      return errorMessage == null;
   }

   public String GetErrorMessage() {
      return errorMessage;
   }

   public int getScore(string word) {
     // return the score of the word no matter what it is
   }
}


Array sort-rank class I had a task where I was supposed to write a solution to sort entries(Name, score - in an array/list) in order of score and generate ranking based on higher score to lower scores.

I tried doing that, code is working what it was supposed to do but it seems very ambiguous and tightly coupled, I can't even write a unit test for it.

Could someone have a look and suggest.

I have pasted main method class, If anyone wants me to paste other relevent classes as well so do ask me please.

Any suggestion(s)/workaround(s) would be much appreciated.

public class JournalAnalyzer {
    final static int LENGTH_OF_JOURNAL = 5; //Length Journal array
    static int count = 0;
    static Integer ranking = 1;
    static Journal[] journals = new Journal[LENGTH_OF_JOURNAL];
    static ArrayList<Journal> list = new ArrayList<Journal>();
    static Object[] scoreList = new Object[LENGTH_OF_JOURNAL];
    static String journalName;
    static Double journalScore;
    static Integer journalRank;
    static Boolean journalReview;

private static void initData(Journal[] journals) {
    journals[0] = new Journal(0, "Journal A", 5.6, false);
    journals[1] = new Journal(0, "Journal B", 2.6, false);
    journals[2] = new Journal(0, "Journal C", 3.2, false);
    journals[3] = new Journal(0, "Journal D", 4.1, false);
    journals[4] = new Journal(0, "Journal E", 1.6, false);
}

private static int fillJournalList(int count, Journal[] journals,
        int LENGTH_OF_JOURNAL, ArrayList<Journal> list) {

    for (int i = 0; i < LENGTH_OF_JOURNAL; i++) {
        journalRank = new Integer(journals[i].getRank());
        journalName = new String(journals[i].getJournal());
        journalScore = new Double(journals[i].getScore());
        journalReview = new Boolean(journals[i].getIsReviewed());

        if (!journalReview) {
            Journal value = new Journal(journalRank, journalName,
                    journalScore, journalReview);
            list.add(value);
            count++;
        } else {
            continue;
        }
    }
    return count;
}

private static void GenerateRanking(Integer ranking, int count,
        ArrayList<Journal> list, Object[] scoreList) {
    Double scoreComparatorOne;
    Double scoreComparatorTwo;

    Comparator<Journal> scores = new SortScore();
    Collections.sort(list, scores);

    for (int i = 0; i < count; i++) {
        scoreComparatorOne = (list.get(i)).getScore();
        if (i == 0) {
            scoreComparatorTwo = (list.get(i)).getScore();
        } else {
            scoreComparatorTwo = (list.get(i - 1)).getScore();
        }
        if (scoreComparatorOne.equals(scoreComparatorTwo)) {
            if (ranking == 0) {
                ranking += 1;
            } else {
                ranking = ranking;
            }
        } else {
            ranking = i + 1;
        }

        (list.get(i)).setRank(ranking);
        scoreList[i] = (list.get(i));

    }
}

private static void displayJournal(int counter, Object[] scoreList) {
    JournalOutput printer = new JournalOutput();
    printer.display(count, scoreList);
    System.exit(0);
}

public static void main(String[] args) throws IOException {
    initData(journals);
    count = fillJournalList(count, journals, LENGTH_OF_JOURNAL, list);
    GenerateRanking(ranking, count, list, scoreList);
    displayJournal(count, scoreList);
}
}

My question is how to minimize and simplify this code so that I can easily write a unit test for this class

You could change this:

for (int i = 0; i < count; i++) {
        scoreComparatorOne = (list.get(i)).getScore();
        if (i == 0) {
            scoreComparatorTwo = (list.get(i)).getScore();
        } else {
            scoreComparatorTwo = (list.get(i - 1)).getScore();
        }
        if (scoreComparatorOne.equals(scoreComparatorTwo)) {
            if (ranking == 0) {
                ranking += 1;
            } else {
                ranking = ranking;
            }
        } else {
            ranking = i + 1;
        }
    (list.get(i)).setRank(ranking);
    scoreList[i] = (list.get(i));
 }

Theres no need to verify all the time this conditional:

if (i == 0) {
                scoreComparatorTwo = (list.get(i)).getScore();
            } else {
                scoreComparatorTwo = (list.get(i - 1)).getScore();
            }

inside the loop. Since i it will be equal to 0 only once.

Modifying  to:

...
Double scoreComparatorOne = (list.get(0)).getScore();
Double scoreComparatorTwo = (list.get(0)).getScore();
ranking = getRating(scoreComparatorOne,scoreComparatorTwo,ranking,0);
(list.get(0)).setRank(ranking);
scoreList[0] = (list.get(0));

for (int i = 1; i < count; i++){
        scoreComparatorOne = (list.get(i)).getScore();
        scoreComparatorTwo = (list.get(i - 1)).getScore();
        ranking = getRating(scoreComparatorOne,scoreComparatorTwo,ranking,i);
        (list.get(i)).setRank(ranking);
        scoreList[i] = (list.get(i));
}

where getRating is:

 public Integer getRating (Double sCOne, Double sCTwo, Integer ranking, int i)
    {
           if(sCOne.equals(sCTwo))
              return (ranking == 0) ? 1 : ranking;
           else return i + 1;
    }

also you can turn your fillJournalList into (but this is not a major problem):

private static int fillJournalList(Journal[] journals,
    int LENGTH_OF_JOURNAL, ArrayList<Journal> list) {

for (int i = 0; i < LENGTH_OF_JOURNAL; i++){

    if (!journals[i].getIsReviewed())
        list.add(new Journal(new Integer(journals[i].getRank(),
                              new String(journals[i].getJournal(),
                              new Double(journals[i].getScore(),
                              false);

return list.size();

}

This way you only create the new Objects when you will insert them. Not in every for iteration. With the above modification there no need to this:

public class JournalAnalyzer {
    ...
    static String journalName;
    static Double journalScore;
    static Integer journalRank;
    static Boolean journalReview;

You can remove all four variables.


Eliminate redundancy from session authentication I have a session authentication script and it works, but I can't find a way to eliminate the redundancy where the two database queries are. One checks if the cookie has a corresponding entry, and the other retrieves user information (if the user is logged on) based on the cookie.

$con = new Database();
if(isset($_COOKIE["session"])) {
    $stmt = $con->query("select null from `sessions` where session_id like :1 and ip_address like :2 and user_agent like :3",
        $_COOKIE["session"],
        $_SERVER["REMOTE_ADDR"],
        $_SERVER["HTTP_USER_AGENT"]
    );
    if($stmt->rowCount() === 1) { // Session is valid
        $stmt = $con->query(
            "select users.user_id, users.nickname, users.email, users.analytic_bool, users.chat_bool,".
            "users.news_bool, users.custom_bool, users.entry_time from `sessions`, `users` ".
            "where users.user_id like sessions.user_id and sessions.session_id like :1 and sessions.ip_address ".
            "like :2 and sessions.user_agent like :3",
                $_COOKIE["session"],
                $_SERVER["REMOTE_ADDR"],
                $_SERVER["HTTP_USER_AGENT"]
        ); // Set session data
        $_SESSION = $stmt->fetchAll()[0];
        $_SESSION["level_name"] = ucfirst(get_level_name($_SESSION["level"]));
    } else generate_new_session();
} else generate_new_session();

Note: The Database(), generate_new_session(), and get_level_name() have not been included because they are not relevant.

By the second query, I already know that the session is valid so there is no need to compare the values all over again. I just can't think of an alternative.

Your two queries are essentially the same, except the second query is also joining to the users table.  This suggests that you might be able to remove the first query and simply execute the second query.

OUTER JOIN

Since you've said that it is possible for a client to have an active session, but no user associated with it, then you would need to to use an outer join, which mysql seems to support, to connect to the users table.  The explanation in the documentation isn't overly easy to follow, but essentially, when you do a normal join results are only returned if all the join criteria are met.  If you do an outer join, then results from the outer joined table (users) will be returned if present, otherwise columns from that table will be returned as null.

You can then determine if a user has a session (a row is returned) and if they're logged on (users columns have values) from a single query.


Recursive math expression eval It has been very hard to use recursion, but I think that it made the code shorter and cleaner.

import doctest
import operator as op

START_FOR_SYMBOLS = {'+': 0,
                     '*': 1,
                     '/':1,
                     '-':0
                     }

OP_FOR_SYMBOL = {'+': op.add,
                 '*': op.mul,
                 '/': op.truediv,
                 '-': op.sub
                 }

def innermost_parens(text):
    """
    Returns the text inside the innermost parenthesis.

    >>> innermost_parens("1 + (2 * (4 - 1))")
    '4 - 1'
    >>> innermost_parens("1 + (2 * (4 * (2 + (8 * 7)) - 1))")
    '8 * 7'
    """
    if not '(' in text and not ')' in text:
        return text
    open_ = text.index('(')
    close_ = text.rindex(')')
    return innermost_parens(text[open_+1:close_])

def polish_eval(expr,start=None):
    """
    Unlimited polish eval, works for any number of arguments.

    >>> polish_eval('+ 4 1 6')
    11.0
    >>> polish_eval('* 4 5 5')
    100.0
    """
    tokens = expr.split(' ')
    if start is None:
        start = START_FOR_SYMBOLS[tokens[0]]
    if len(tokens) == 1:
        return start
    return polish_eval(' '.join(tokens[:-1]),
                       start = OP_FOR_SYMBOL[tokens[0]](start,float(tokens[-1]))
                       )

def infix_eval(expr):
    """
    Reduced infix eval, only works with 2 numbers.

    >>> infix_eval('9 + 4')
    13.0
    >>> infix_eval('2 * -6')
    -12.0
    """
    a, oper, b = expr.split()
    return OP_FOR_SYMBOL[oper](float(a),float(b))

def full_eval(expr, eval_type):
    """
    Evals by the rules of eval_type starting from the inner
    parenthesis.

    >>> full_eval("(* 4 5 (+ 4 1))", polish_eval)
    100.0
    >>> full_eval("(* 4 (/ 10))", polish_eval)
    0.4
    >>> full_eval("(1 + (5 * 2))", infix_eval)
    11.0
    """
    if len(expr.split(' ')) == 1:
        return float(expr)
    inn = innermost_parens(expr)
    new_expr = expr.replace('('+str(inn)+')',str(eval_type(inn)))
    return full_eval(new_expr, eval_type)

def interface():
    which_expr = input("Polish or infix? ")
    if 'polish' in which_expr.lower():
        evaller = lambda expr: full_eval(expr, polish_eval)
    else:
        evaller = lambda expr: full_eval(expr, infix_eval)
    while True:
        result = evaller(input('> '))
        print(result)

if __name__ == "__main__":
    doctest.testmod()
    interface()

Good job on adding the test suites; they make testing code extremely easy.

Recursion often results in a clean solution. However, I think that this problem can be solved just as well with an iterative approach.

The key to innermost_parens is to return the text between the last ( and the first ). Recursion is a viable solution, but I find that this for loop does the job better and in a cleaner way:

def innermost_parens(text):
    """
    Returns the innermost parenthesis.

    >>> innermost_parens('* 1 2')
    '* 1 2'
    >>> innermost_parens("1 + (2 * (4 - 1))")
    '(4 - 1)'
    >>> innermost_parens("1 + (2 * (4 * (2 + (8 * 7)) - 1))")
    '(8 * 7)'
    >>> innermost_parens("(1 + 2) * (3 + 4)")
    '(1 + 2)'
    """
    start = 0
    end = len(text)
    for (i, c) in enumerate(text):
        if c == '(':
            start = i
        if c == ')':
            end = i + 1
            break
    return text[start:end]

Take note that this function will return the first innermost expression with brackets. The reason for this will be clear later.

As for polish_eval, your program doesn't follow the standard Polish notation. In the standard Polish notation you can only perform operations on two numbers. It is also unclear what 'start' does. I modified your code to do the above and added tests for subtraction and division:

def polish_eval(expr):
    """
    Reduced Polish notation.

    >>> polish_eval('+ 4 1')
    5.0
    >>> polish_eval('* 4 5')
    20.0
    >>> polish_eval('- 20 3')
    17.0
    >>> polish_eval('/ 20 5')
    4.0
    """
    oper, a, b = expr.split()
    return OP_FOR_SYMBOL[oper](float(a), float(b))

I'll leave the documentation of the Polish notation to you. No changes were made to infix_eval.

Finally, we're on to full_eval. Since there's been a major change in the notation we support, we'll have to rewrite the code. First, to start off with the tests:

>>> full_eval("* (* 4 5) (+ 4 1)", polish_eval)
100.0
>>> full_eval("* 4 (/ 1 10)", polish_eval)
0.4
>>> full_eval("1 + (5 * 2)", infix_eval)
11.0

Rewriting Polish notation without brackets would require the use of a stack. This will not fit nicely in your full_eval function, so I will not implement the notation without brackets. In this function, the use of recursion is unnecessary and a while loop will fit the problem nicely.

def full_eval(expr, eval_type):
    """
    Evals by the rules of eval_type starting from the inner
    parenthesis.

    >>> full_eval("* (* 4 5) (+ 4 1)", polish_eval)
    100.0
    >>> full_eval("* 4 (/ 1 10)", polish_eval)
    0.4
    >>> full_eval("1 + (5 * 2)", infix_eval)
    11.0
    """
    while len(expr.split()) != 1:
        inn = innermost_parens(expr)
        result = eval_type(inn.strip('(').strip(')').strip())
        expr = expr.replace(inn, str(result))
    return float(expr)

Before I evaluate inn, I first strip off the brackets which enclose the expression, as well as any remaining whitespace.

The reason why I wrote innermost_parens as shown above is to allow for the easy replacement of the expression. Here's a trace of an expression in Polish:

  * (/ 9 3) (+ (/ 3 5) (- 20 5))
= * (/ 9 3) (+ 0.6 (- 20 5))      # '(/ 3 5)' -> '0.6'
= * (/ 9 3) (+ 0.6 15.0)          # '(- 20 5)' -> '15.0'
= * 3.0 (+ 0.6 15.0)              # '(/ 9 3)' -> '3.0'
= * 3.0 15.6                      # '(+ 0.6 15.0)' -> '15.6'
= 46.8                            # '* 3.0 15.6' -> '46.8'

and the same expression in infix:

  (9 / 3) * ((3 / 5) + (20 - 5))
= (9 / 3) * (0.6 + (20 - 5))      # '(3 / 5)' -> '0.6'
= (9 / 3) * (0.6 + 15.0)          # '(20 - 5)' -> '15.0'
= 3.0 * (0.6 + 15.0)              # '(9 / 3)' -> '3.0'
= 3.0 * 15.6                      # '(0.6 + 15.0)' -> '15.6'
= 46.8                            # '3.0 * 15.6' -> '46.8'

Extensions:

Write your program such that it can parse Polish notation without brackets. That's what Polish notation is invented for.
Handle edge cases such as ((* 3 15)) (Polish).
Write test suites to ensure that the expressions are evaluated correctly. While addition and multiplication is commutative, subtraction and division are not, and the order of the numbers matter.
Follow suggestions in l0b0's answer, which are cosmetic. I did not make any cosmetic suggestions as I thought that it was more important to settle the logic of the program first.
Have your program create an abstract syntax tree instead of doing string operations (such as replacing innermost expression with its value).

Feel free to ask questions in the comments.


Quicker way to go through 550 variables with very similar code I'm hoping to figure out a faster way to write this before I resign myself to copying and pasting 500 times.  I have code here that needs to do tons of vlookup matches.  The only thing that changes are two variables each time.

First part is just establishing variables and a count of the rows I need to loop through for each thing:

Sub GetInfo()
    Dim v As Long, vWSs As Variant, Mrange As Range, Vrange As Range
    Dim wsMaster As Worksheet: Set wsMaster = Workbooks("LBImportMacroTemplate.xlsm").Worksheets("MasterTab")
    Dim mf_iA_TEXT As String: mf_iA_TEXT = "iA"
    Dim mf_pLN_TEXT As String: mf_pLN_TEXT = "pLN"
    'ET CETERA, MANY MORE STRING VARIABLES LIKE THIS
    vWSs = Array("B", "E", "L", "I", "T")
    With Workbooks("LBImportMacroTemplate.xlsm")
        Set Mrange = Nothing: Set Vrange = Nothing
'
With ActiveSheet
    lastrow = .Cells(.Rows.Count, "A").End(xlUp).Row
    MsgBox lastrow
End With

Now, the main code:

'iA
For v = LBound(vWSs) To UBound(vWSs)
    If CBool(Application.CountIf(.Sheets(vWSs(v)).Range("A2:ZA2"), mf_iA_TEXT)) Then
        Set Mrange = .Sheets(vWSs(v)).Range("A2:ZA2")
        Set Vrange = .Sheets(vWSs(v)).Range("A:ZA")
        mf_iA = Application.Match(mf_iA_TEXT, Mrange, 0)
    For i = 2 To lastrow
            wsMaster.Cells(i, 2) = Application.VLookup(wsMaster.Cells(i, 1), Vrange, mf_iA, 0)
    Next i
        Exit For
    End If
Next v

Then this part repeats again, with two different variables (the mf_x and mf_x_TEXT are the constant naming things but something else will be in place of the x every time.

'pLN
For v = LBound(vWSs) To UBound(vWSs)
    If CBool(Application.CountIf(.Sheets(vWSs(v)).Range("A2:ZA2"), mf_pLN_TEXT)) Then
        Set Mrange = .Sheets(vWSs(v)).Range("A2:ZA2")
        Set Vrange = .Sheets(vWSs(v)).Range("A:ZA")
        mf_pLN = Application.Match(mf_pLN_TEXT, Mrange, 0)
    For i = 2 To lastrow
            wsMaster.Cells(i, 3) = Application.VLookup(wsMaster.Cells(i, 1), Vrange, mf_pLN, 0)
    Next i
        Exit For
    End If
Next v

And repeats and repeats...500+ times.  Is there a better way to do this?  Please teach me if so.

EDIT:  Here's a better explanation of the function, it's in a spreadsheet so .  The MasterTab looks like this

sLN iA  pLN iC  iB  aLN
100
101
102
103
104
105
106
107
108
109
110

those are the headings.  Matching heading can be found on five other tabs:  B, E, L, I, and T.

The code above does a vlookup match to search all 5 tabs for a column with the right name (as defined by the mf_x_TEXT variable).  Then the spreadsheet looks like this:

sLN iA  pLN iC  iB  aLN
100 4444    a   etc etc etc
101 4444    b
102 4746    c
103 8844    d
104 8383    e
105 3456    f
106 8836    g
107 2342    h
108 8888    i
109 1111    j
110 1123    k

like I said, it works, but there are 500 more columns where that came from and I'm just trying to shorten my work so I don't have to copy/paste/click so much.  Hope that helps, let me know if I can provide anything else.

This is an odd code review for me. I could talk about giving your variables meaningful names, proper indentation, avoiding activate and select, extracting functions/subs, looping over ranges using a For Each, etc. , but I'm not going to talk about any of that. Because you don't need code to do this at all.

With some patience, you should be able to craft a native excel formula that does all of this. It will be less readable than code, but it will also be exponentially faster.

Let's start here:

wsMaster.Cells(i, 3) = Application.VLookup(wsMaster.Cells(i, 1), Vrange, mf_pLN, 0)

and translate this into a formula.

=VLOOKUP("[MasterTab]!$A2","[Sheet2]!A:ZA",mf_Pln,FALSE)

We obviously can't use a variable there, so let's take a look at how we get that value.

mf_pLN = Application.Match(mf_pLN_TEXT, Mrange, 0)

Okay, so let's nest that into our function.

 =VLOOKUP("[MasterTab]!$A2","[Sheet2]!A:ZA",Match(A$1, "[Sheet2]!$A$2:$ZA$2", 0),FALSE)

Take careful note of the dollar signs. They make the reference absolute, so the reference cell doesn't change as the formula gets copied into new cells. Those without a $ are relative and will "move with" the cells.

Now, just nest this inside of your COUNTIF and you have a formula ready to go into your worksheet. Fill right, then down.

Of course, this works on a single specified worksheet. You'll still need to loop through all of the worksheets to actually build your final formula. Replacing the target sheet name accordingly.

It's going to be nested deep, but a considerably faster solution using formulas versus VBA.


How can I make my solution to "Climbing the Leaderboard" not time out? I tried to solve the following challenge and optimize the solution but my code still times out. My code is below.

Problem statement

  Climbing the Leaderbord

  Alice is playing an arcade game and wants to climb to the top of the
  leaderboard and wants to track her ranking. The game uses Dense
  Ranking,
  so its leaderboard works like this:

  The player with the highest score is ranked number 1 on the leaderboard.
  Players who have equal scores receive the same ranking number, and the next player(s) receive the immediately following ranking number.

  For example, the four players on the leaderboard have high scores of
  100, 90, 90, and 80. Those players will have ranks 1, 2, 2, and 3,
  respectively. If Alice's scores are 70, 80 and 105, her rankings after
  each game are 4th, 3rd and 1st.

  Function Description

  Complete the climbingLeaderboard function in the editor below. It
  should return an integer array where each element \$res[j]\$
  represents Alice's rank after the \$j^{th}\$ game.

  climbingLeaderboard has the following parameter(s):

  scores: an array of integers that represent leaderboard scores
  alice: an array of integers that represent Alice's scores

  Input Format

  The first line contains an integer \$n\$, the number of players on the
  leaderboard. The next line contains \$n\$ space-separated integers
  \$scores[i]\$, the leaderboard scores in decreasing order. The next
  line contains an integer, \$m\$, denoting the number games Alice
  plays. The last line contains \$m\$ space-separated integers
  \$alice[j]\$, the game scores.

  Constraints

  \$1 \le n \le 2 \times 10^5\$
  \$1 \le m \le 2 \times 10^5\$
  \$0 \le scores[i] \le 10^9\$ for \$0 \le i \lt n\$
  \$0 \le alice[j] \le 10^9\$ for \$0 \le j \lt m\$
  The existing leaderboard, \$scores\$, is in descending order.
  Alice's scores, \$alice\$, are in ascending order.

  Subtask

  For 60% of the maximum score:

  \$1 \le n \le 200\$
  \$1 \le m \le 200\$

  Output Format

  Print \$m\$ integers. The \$j^{th}\$ integer should indicate Alice's
  rank after playing the \$j^{th}\$ game.

  From Climbing the Leaderboard on HackerRank.com.

My code

The code simply starts with climbingLeaderboard method below which delete all similar values in the scores array,
after that it start with Alice score and using binary search it return the correct position.

int getPosition(vector<int> scores,int value,int start,int end){

    int middle = (start+end)/2;

    if(scores[start]<value){
        return start;
    }else if(scores[end]>value){
        return end+1;
    }

    else if(scores[middle]==value){
        return middle;
    }
    else if(start==end){
        if(scores[start]>value){
            return start+1;
        }else{
            return start;
        }
    }

    else if(scores[middle]>value){
        return getPosition(scores,value,middle+1,end);
    }else{
        return getPosition(scores,value,start,middle-1);
    }

}
/////
/////

vector<int> climbingLeaderboard(vector<int> scores, vector<int> alice) {

    int i=1;
    while(1){
        if(i == scores.size()){
            break;
        }
        if(scores[i]==scores[i-1]){
            scores.erase(scores.begin() + i);
            continue;
        }
        i++;
    }
    vector<int> results;
    int endValue = scores.size()-1;

    for(int j=0;j<alice.size();j++){
        endValue = getPosition(scores,alice[j],0,endValue);
        results.insert(results.end(),endValue+1);
    }

    return results;

    }

Any ideas? How can I make it not time out?

\$\DeclareMathOperator{\Oh}{O}\$Frankly, the code in the HackerRank editor is a large mess.  It is promoting crap like #include <bits/stdc++.h> and using namespace std;, and the code contains many problems: int for traversing a std::vector, copying containers around, etc.  Not to mention bad practices like i++, explicitly calling fout.close(), not using standard algorithm, etc.  It really shouldn't be like that.

In your code, the whole getPosition function is redundant.  Use std::lower_bound instead.  And since std::vector::erase is \$\Oh(n)\$, the code is \$\Oh(n^2)\$, which is unnecessary inefficient.

The problem is very simple and can be finished in several lines without sacrificing readability:

using score_t = long;
using rank_t = long;

// scores is passed by value to take advantage of possible optimization.
rank_t get_rank(std::vector<score_t> scores, score_t alice_score)
{
    scores.erase(std::unique(scores.begin(), scores.end()), scores.end());

    auto it = std::lower_bound(scores.begin(), scores.end(), alice_score, std::greater<>{});
    return it - scores.begin() + 1;
}

The code is \$\Oh(n)\$ and I don't think you can do better.  Note that each score of Alice's is independent, so it makes no sense to process them together in a function.  I used long because the problem seems to require numbers as large as \$10^9\$.  scores will be modified in the function, so instead of making a copy manually, we let the compiler do so for us in the parameter list.  This enables possible optimization opportunities.

Here, we used two standard algorithms:

std::unique, which "removes" adjacent equal elements.  Standard algorithms cannot change the size of scores via iterators, so std::unique makes sure that the first \$N\$ elements are the result, where \$N\$ is the number of elements in the result.  The rest of the elements are placed in a valid but otherwise unspecified state.  Then, we call erase to erase these garbage elements.  This is also known as the remove-erase idiom.
std::lower_bound, which performs a binary search and returns the first element that compares not "less" than the provided value.  By default, "less" is defined by <, thus operating on an ascending sequence.  In this case, we use std::greater<> to define "less" by >, so that std::lower_bound is adapted to work on a descending sequence.

Now let's go through your code: (regardless of whether the code is what hackerrank gives you)

int getPosition(vector<int> scores,int value,int start,int end){

As I said, this function is provided by the standard library and you shouldn't be reinventing your own without a good reason.  If you want to see how the standard version looks like, there is an example implementation at cppreference.

vector<int> climbingLeaderboard(vector<int> scores, vector<int> alice) {

As I said before, this function is illogical.  It should handle one Alice-score at a time.  Also, climbingLeaderboard isn't really a good function name.

Passing scores by value is justified because you modify it in the functions, but passing alice by value introduces an unnecessary copy.  It should be passed by const reference instead.  And int is not only a magic type, but also a type that is not sufficiently large here.

int i=1;
while(1){
    if(i == scores.size()){
        break;
    }
    if(scores[i]==scores[i-1]){
        scores.erase(scores.begin() + i);
        continue;
    }
    i++;
}

Space.  Also, I am not sure what you are trying to achieve with the while (1) loop here: you are obfuscating the code by refusing to place the condition in the proper place.  And int isn't the correct type to use here.  Use size_type (or at least a named type).  ++i should be used instead of i++ in a discarded value expression.

And you are essentially reimplementing the \$\Oh(n)\$ std::unique in a \$\Oh(n^2)\$ way.  Just use the standard algorithm and avoid reinventing the wheel.

vector<int> results;
int endValue = scores.size()-1;

In this particular case, we know that !scores.empty(), but storing the result in a int is still inadvisable.  Use auto or std::vector<int>::size_type.

for(int j=0;j<alice.size();j++){
    endValue = getPosition(scores,alice[j],0,endValue);
    results.insert(results.end(),endValue+1);
}

Again, space.  And ++j not j++.  And the third line is just

results.push_back(endValue + 1);

There is no reason to use insert + iterator here.

    return results;

    }

Is the weird indentation a copy-paste problem?


Code Kata - Berlin Clock To improve my programming style, I have been writing solutions for popular code katas. I have written a partial solution for the Berlin Clock kata and have been trying to follow the SOLID Principles. Can anyone critique the code and suggest improvements?

public class BerlinClock {

    private static List<TimeUnit> timeUnits = new ArrayList<TimeUnit>();

    public static void main(String[] args) {

        timeUnits.add(new Second());
        timeUnits.add(new Hour());
        timeUnits.add(new Minute());

        System.out.println(getTime("13:17:01"));
    }

    public static String getTime(String time) {

        StringBuilder timeSB = new StringBuilder();
        String[] timeElements = time.split(":");

        int i=0;
        int j=2;

        for(TimeUnit timeUnit : timeUnits){

            for(String timeElement : timeElements){
                timeSB.append(timeUnits.get(i).getLamps(Integer.parseInt(timeElements[j]))).append("\n");
                i++;
                j--;
                break;
            }
        }

        return timeSB.toString();
    }
}

TimeUnit

public interface TimeUnit {

    public String getLamps(int unit);

}

Hour

public class Hour implements TimeUnit {

    private static final String ALL_LIGHTS_OFF = "OOOO";
    private static final String RED_LIGHT = "R";
    private static final int MULTIPLE_OF_FIVE = 5;

    @Override
    public String getLamps(int hrs) {

        return getLampsForHoursMultiplesOfFive(hrs) + "\n" + getLampsForSingleHours(hrs);
    }

    public String getLampsForHoursMultiplesOfFive(int hours) {

        StringBuilder lamps = new StringBuilder(ALL_LIGHTS_OFF);

        for (int i = 0; i < (hours / MULTIPLE_OF_FIVE); i++) {
            lamps.replace(i, i + 1, RED_LIGHT);
        }
        return lamps.toString();
    }

    public String getLampsForSingleHours(int hours) {

        StringBuilder lamps = new StringBuilder(ALL_LIGHTS_OFF);

        for (int i = 0; i < (hours % MULTIPLE_OF_FIVE); i++) {  //dont do calculaion ever loop, do outside loop
            lamps.replace(i, i + 1, RED_LIGHT);
        }
        return lamps.toString();
    }
}

Minute

public class Minute implements TimeUnit {

    private static final String RED_LIGHT = "R";
    private static final String YELLOW_LIGHT = "Y";
    private static final String ALL_FOUR_LIGHTS_OFF = "OOOO";
    private static final String ALL_ELEVEN_LIGHTS_OFF = "OOOOOOOOOOO";

    @Override
    public String getLamps(int minutes) {

        int minutesDividedBy5 = minutes / 5;
        int minutesModulus5 = minutes % 5;

        return getLampsForMinutesMultiplesOfFive(minutesDividedBy5) + "\n" + getLampsForSingleMinutes(minutesModulus5);
    }

    private String getLampsForMinutesMultiplesOfFive(int minutes) {

        StringBuilder lamps = new StringBuilder(ALL_ELEVEN_LIGHTS_OFF);

        for (int i = 0; i < minutes; i++) {
            if (0 == (i + 1) % 3) {
                lamps.replace(i, i + 1, RED_LIGHT);
            } else {
                lamps.replace(i, i + 1, YELLOW_LIGHT);
            }
        }
        return lamps.toString();
    }

    private String getLampsForSingleMinutes(int minutes) {

        StringBuilder lamps = new StringBuilder(ALL_FOUR_LIGHTS_OFF);

        for (int i = 0; i < minutes; i++) {
            lamps.replace(i, i + 1, YELLOW_LIGHT);
        }
        return lamps.toString();
    }
}

Second

public class Second implements TimeUnit {

    @Override
    public String getLamps(int seconds) {

        if (0 == seconds%2) {
            return "Y";
        }

        return "O";
    }
}

That's a lot of code for something that could be done with a single function. The number of operations required to determine the output is relatively small, so writing a whole TimeUnit class hierarchy for it is overkill. Keeping your code short and simple makes it easier to understand, which helps preventing (or spotting) bugs.

If you do want to write a utility class, then a single, configurable TimeUnit class is sufficient. The only difference between the time units are lamp color and number of lamps per row, which can be passed in via the TimeUnit constructor.

Instead of passing strings around, why not use a built-in date/time type? That, or a set of 3 integers. That will clearly communicate how the function is to be used. A string is more ambiguous: should I pass in "15:30:05", "3:30:05PM", or some other format?

The 'S' in SOLID stands for Single responsibility. Let another function do the parsing. Your function should focus on the time-to-BerlinClock-display conversion. That's also related to 'I': Interface segregation. It's better to have several classes (or functions, in this case) that each do one thing well, than one big class/function that does everything, but turns out to be very inflexible when you only need part of that 'everything'.

Remember, KISS: Keep It Short and Simple. Also, DRY: Don't Repeat Yourself.


Avoiding code duplication in multiple except blocks in Logger class Context:

A logger records events which contain an area, a level, a message and an option indicating that the source replaces another one.

The logger attempts to send the message through HTTP, and on failure, saves it locally. If HTTP service times out, the logger stops attempting using HTTP and stores the message locally directly.

Concern:

I'm concerned with code duplication at the end of the method. The same call is repeated three times. In my opinion:

Creating a function within the method would be an overkill.
Creating a separate class to encompass the four elements and pass an instance of this class would be an overkill as well.
Moving the call to the end and using return would complicate the logic.

What are my options?

class Logger():
    ...
    def _log(self, area, level, message, replacing=None):
        ...
        if self._timedOut:
            self._logToFile(area, level, message, replacing) # ← This line...
            return

        try:
            self._post(json)
        except urllib.error.HTTPError:
            self._logToFile(area, level, message, replacing) # ← ... is duplicated here...
        except socket.timeout:
            self._logToFile(area, level, message, replacing) # ← ... and here.
            self._timedOut = True

Method decomposition looks like the way to go here:

def _try_post(self, json):
    """Try posting the message (unless this timed out previously).
    Return True if successful, False otherwise.

    """
    if not self._timedOut:
        try:
            self._post(json)
            return True
        except urllib.error.HTTPError:
            pass
        except socket.timeout:
            self._timedOut = True
    return False


Most frequent integer in a list Here was my attempt at finding the most frequent integers in a list, I feel like there are better ways to do this and I spent a great deal of time coming up with this solution, so please point me in the correct direction!

'''
Created on Jan 27, 2018
Problem statement: Find the most frequently occuring integer in a list and its occurence;
    input: 1 1 2 3 2 1 2
    output: [(1,3), (2, 3)]

@author: Anonymous3.1415
'''
from collections import Counter

def integer_frequency(int_list):
    int_freq = Counter()
    for i in int_list:
        int_freq[i] += 1
    cnt = 0
    values = sorted(int_freq.values())
    values.reverse()
    for x in values:
        if x == values[0]:
            cnt += 1
        else:
            break
    return int_freq.most_common(cnt)

int_list = list(map(int, raw_input().strip().split(' ')))
most_frequent = integer_frequency(int_list)
print(most_frequent)

You could use itertools.takewhile to keep on taking tuples until they are not the same count anymore:

from collections import Counter
from itertools import takewhile

def integer_frequency(integers):
    int_freq = Counter(integers)
    most_common = int_freq.most_common(1)[0]
    return list(takewhile(lambda x: x[1] == most_common[1],
                          int_freq.most_common()))

I also renamed your integer_list to integers, because it can be any iterable of integers.

Alternatively, you could use itertools.groupby to group the tuples by their frequency and return the group with the highest frequency, as @Gareth Reese has suggested in the comments:

from collections import Counter
from itertools import groupby

def integer_frequency(integers):
    return list(next(groupby(Counter(integers).most_common(),
                             key=lambda x: x[1]))[1])

This approach is consistently faster (by a small amount):

However, both beat your algorithm by quite a lot:


Trie Data Structure Implementation in C++11 using Smart Pointers -- follow-up Link to my first question.

Link to my latest source code.

I followed @JDługosz recommendations. How does it look? Do you have any further recommendations? Is it better (if possible) to replace shared_ptr with unique_ptr? How could someone extend it to use the Unicode character set?

#pragma once

#include <iostream>
#include <memory>
#include <string>

namespace forest {
    class trie {
    private:
        struct Node {
            std::shared_ptr<Node> children[26];
            bool end = false;
        };
        std::shared_ptr<Node> root = std::make_shared<Node>();
    public:
        void insert(const std::string & key) {
            std::shared_ptr<Node> n = root;
            for (auto c : key) {
                int index = c - 'a';
                auto& slot = n->children[index];
                if (!slot) slot = std::make_shared<Node>();
                n = slot;
            }
            n->end = true;
        }
        bool search(const std::string & key) {
            std::shared_ptr<Node> n = root;
            for (auto c : key) {
                int index = c - 'a';
                auto& slot = n->children[index];
                if (!slot) return false;
                n = slot;
            }
            return n && n->end;
        }
    };
}

Input sanitization

Your functions take a std::string as parameter, as such, they should be "well behaved" for any possible std::string passed to them. Note that well behaved does not mean that it has to "work", just that it should not break anything.

Specifically, what would happen if I passed the string "Hello" to that function? 'H' - 'a' is -25, ruh roh!

There's a few different ways to tackle this.

You could convert all upper-case letters to lower case, but that does not fix punctuation marks, spaces and numbers. I also personally think that wether the trie is case-sensitive or not should be the user's problem, not the trie itself.
You could bail out of the function if a unhandled character is hit.
Just expand children to 256 instead of 26 so that all legal values of char are handled properly. Sure your trie is going to be 5 times as big, but that's a fairly minor detail since it grows logarithmically.

edit: This last approach also makes the trie operate on raw data instead of characters, which makes it encoding-agnostic (which gives you your unicode support)

Avoid using shared_ptr unless absolutely necessary

shared pointers are clunky and heavy affairs. On top of this, using them sends a message to people reading the code: Ownership of the object is potentially shared between multiple owners, which is simply not the case here.

in your case, a std::unique_ptr<> is absolutely fine.

Your root does not need to be dynamically allocated

It's created at construction unconditionally, and destroyed at destruction unconditionally. On top of that, it does not make use of type erasure (polymorphism). As such, there's no reason for it not to be a normal member of trie.

std::shared_ptr<Node> root = std::make_shared<Node>();

becomes:

Node root;

This will require you to change your search and insert functions so that the first line becomes:

const Node* n = &root;

But that's fine because that's preferable once you move to unique_ptr anyways.

Edit: On that note:

Raw pointers are not evil

std::shared_ptr<Node> n = root;

We tend to teach people that "you should never use raw pointers". But I find that extremely naive. The real rule is "You should never have raw pointers with ownership".

There is absolutely nothing wrong with using raw pointers as long as it's understood that:

The pointer only refers to the pointed object without owning it.
The lifetime of the pointer assignment to the object is fully enclosed by the lifetime of whatever DOES own the object.

In your code using shared_ptr, using the following would have been 100% ok, and much better in my opinion:

const Node* n = root.get();

Mark non-mutating functions as const.

Your search() member function should not alter the trie in any way, so you should mark it as const like this:

bool search(const std::string & key) const {

There's a few subtle advantages for the compiler, but the main one is that if you goof up and accidentally do something that changes the trie, the compiler will tell you.

nitpick: private: is redundant here.

The namespace of a class is private by default. Fun fact: that's the only difference between a class and a struct.

nitpick: redundant null-checking

in your search function, when I read the last line:

return n && n->end;

My impression was "Oh! n can be possibly null in certain cases", which led me to search for the scenario where that might be the case. It's misleading to the reader.

Defensive programming can be useful at times, but this is just excessive.


Adding blank rows for every skipped value in a DataTable So I have a DataTable that returns results within a month to month range when available.  However, when I format the return, I need to still show skipped months as blanks.

I have a working solution, but it's a little verbose and I think it can be done cleaner.

Here's a setup to the problem

Dim startMonth As Date = #5/1/2011#
Dim endMonth As Date = #6/1/2012#
Dim dr As DataRow

Dim dtData As New DataTable
With dtData.Columns
    .Add("MonthDate", GetType(Date))
    .Add("OtherInfo", GetType(String))
    'other columns...
End With

With dtData.Rows
    .Add(#6/1/2011#, "Info")
    .Add(#7/1/2011#, "Info")
    .Add(#7/1/2011#, "Info")
    .Add(#1/1/2012#, "Info")
End With

This part will loop through the date ranges and generate a DataTable with one blank row per possible date

'get all dates
Dim dtAllTimes As DataTable = dtData.Clone()

Do While startMonth <= endMonth
    dr = dtAllTimes.NewRow()
    dr("MonthDate") = startMonth
    dtAllTimes.Rows.Add(dr)
    startMonth = startMonth.AddMonths(1)
Loop

This part will create a blank DataTable with one date per found value

'get used dates
Dim dtUsedTimes As DataTable = dtData.Clone()

For Each usedDate As Date In
        From row As DataRow In dtData.AsEnumerable Select CDate(row("MonthDate")) Distinct
    dr = dtUsedTimes.NewRow()
    dr("MonthDate") = usedDate
    dtUsedTimes.Rows.Add(dr)
Next

Finally I combine those two tables to get the unused dates and add them onto the original data

'subtract used dates from all dates
Dim unusedDates As IEnumerable(Of DataRow)
unusedDates = dtAllTimes.AsEnumerable.Except(dtUsedTimes.AsEnumerable(), DataRowComparer.Default)

'add subtraction back into all
dtData = dtData.AsEnumerable.Union(unusedDates).CopyToDataTable()

Any suggestions would be great.

Update...

Implementing my own custom comparison class helps to shave off a couple lines of code and processing time by not having to compare every column in the entire row against every other column and also helps skip the step of finding used rows since that was only done to put them into a DataTable that could be used with the Default DataRowComparer.

Here's the custom DataRowComparer

Public Class DataRowDateComparer : Implements IEqualityComparer(Of DataRow)

    Public Function Equals(x As DataRow, y As DataRow) As Boolean _
            Implements IEqualityComparer(Of DataRow).Equals
        Return x("MonthDate") = y("MonthDate")
    End Function

    Public Function GetHashCode(obj As DataRow) As Integer _
            Implements IEqualityComparer(Of DataRow).GetHashCode
        Return obj.ToString.GetHashCode
    End Function

End Class

Here's the code after creating a DataTable with every date

'only add rows that aren't already in datatable
dtData = dtData.AsEnumerable _
               .Union(dtAllTimes.AsEnumerable, New DataRowDateComparer) _
               .CopyToDataTable()

Also, sorting by MonthDate in a DataView can help view the results more easily

Dim dv As New DataView(dtData)
dv.Sort = "MonthDate"

You can have fewer lines of code by creating a the list of missing dates in a single Linq query.

The idea is:

Get the list of all dates in the given time period)
Left-Outer-Join this list with the list of given dates
Add the missing dates to the original DataTable

Dim missingDates = From a in Enumerable.Range(0, Int32.Maxvalue)
                   Let c = startMonth.AddMonths(a)
                   Take While c <= endMonth 'create list of all dates'
                   Group Join d in dtData On c Equals d("MonthDate") Into g = Group
                   Where Not g.Any() 'take only the missing ones'
                   Select c

For Each d in missingDates 'and add them to the DataTable'
    dtData.Rows.Add(d, Nothing)
Next

Using Union is also fine IMHO, but then the GetHashCode method of your comparer should return the hash code of the value you actually want to compare:

Public Function GetHashCode(obj As DataRow) As Integer _
        Implements IEqualityComparer(Of DataRow).GetHashCode
    Return obj("MonthDate").GetHashCode
End Function


Check secure image upload I want to upload images to server from browser window. I'm using this function for image upload. Images are uploaded by GD library.

What is your opinion of this code? Is there still a way to upload a malicious file?

<?php
function imageUpload($name, $thumb = false, $max_size = 5242880, $height = null, $width = null, $T__height = 100, $T__width = 100)
{
    if(!isset($_FILES[$name]))
    {
        return false;
    }

    $allowedTypes = array('image/gif', 'image/jpeg', 'image/png', 'image/wbmp');
    $image = &$_FILES[$name];

    if ($image['error'] == 0 && in_array($image['type'], $allowedTypes) && $image['size'] <= $max_size)
    {
        $in = '';
        switch ($image['type'])
        {
            case 'image/gif':
                $in = 'imagecreatefromgif';
                break;
            case 'image/jpeg':
                $in = 'imagecreatefromjpeg';
                break;
            case 'image/png':
                $in = 'imagecreatefrompng';
                break;
            case 'image/wbmp':
                $in = 'imagecreatefromwbmp';
                break;
        }

        if ($in == '')
        {
            return false;
        }

        $src = $in($image['tmp_name']);
        $height = ($height == null || $height <= 0 ? imagesy($src) : $height);
        $width = ($width == null || $width <= 0 ? imagesx($src) : $width);

        $dst = imagecreatetruecolor($width, $height);
        imagecopyresized($dst, $src, 0, 0, 0, 0, $width, $height, imagesx($src), imagesy($src));

        $fileName = '';
        do
        {
            $fileName = makeHash(mt_rand(0, mt_getrandmax()) . microtime(true), $image['tmp_name']);
        }
        while(file_exists('/image/' . $fileName . '.jpg') || ($thumb && file_exists('/thumb/' . $fileName . '.jpg')));
        if ($fileName == '')
        {
            return false;
        }

        imagejpeg($dst, '/image/' . $fileName . '.jpg', 75);

        if ($thumb)
        {
            $dst = imagecreatetruecolor($T__width, $T__height);
            imagecopyresized($dst, $src, 0, 0, 0, 0, $T__width, $T__height, imagesx($src), imagesy($src));
            imagejpeg($dst, '/thumb/' . $fileName . '.jpg', 75);
        }

        imagedestroy($src);
        imagedestroy($dst);

        return $fileName . '.jpg';
    }
}
?>

Is there still a way to upload a malicious file?

I think not, due to these elements:

The check of $image['type'] against an array of various image/* mime types prevents uploading a script instead of an image
The hashing of the filename with makeHash prevents uploading a script with a malicious path to get the file placed somewhere unintended
The check of $image['size'] against a defined maximum prevents uploading an oversized file

  What is your opinion of this code?

Instead of this:

    if ($in == '')
    {
        return false;
    }

It would be better to add a default statement in the switch above it:

    switch ($image['type'])
    {
        // ...
        case 'image/wbmp':
            $in = 'imagecreatefromwbmp';
            break;
        default:
            return false;
    }

This seems pointless to me:

    if ($fileName == '')
    {
        return false;
    }

It seems to me that $fileName will always be set.


Calculator using class templates I've just completed an assignment about class templates in C++. It works fine and produces the correct output. We were given the main function and had to construct and implement the class. I'm wondering if there is a better (more efficient) way to construct and implement this simple calculator class template.

#include <iostream>
#include <string>

using std::cout;
using std::endl;
using std::string;

template <typename T1, typename T2>
class Calculator{
public:
      Calculator(T1 first, T2 second);
      Calculator( const Calculator<T1, T2>& otherCalculator);
      void setValue1(T1 first);
      void setValue2(T2 second);
      T1 add();
      T1 multiply();
 private:
      T1 number1;
      T2 number2;
 };

 template <typename T1, typename T2>
 Calculator<T1, T2>::Calculator(T1 first, T2 second)
      :number1(first), number2(second)
 {}

 template <typename T1, typename T2>
 Calculator<T1, T2>::Calculator(const Calculator<T1, T2>& otherCalculator)
      :number1(otherCalculator.number1), number2(otherCalculator.number2)
 {}

 template <typename T1, typename T2>
 void Calculator<T1, T2>::setValue1(T1 first){
      number1 = first;
 }

 template <typename T1, typename T2>
 void Calculator<T1, T2>::setValue2(T2 second){
      number2 = second;
 }

 template <typename T1, typename T2>
 T1 Calculator<T1, T2>::add(){
      T1 answer = number1 + number2;
      return answer;
 }

template <typename T1, typename T2>
T1 Calculator<T1, T2>::multiply(){
      T1 answer = number1 * number2;
      return answer;
}

int main(){
      Calculator<int,int> simpleCalc(42, 3);
      cout << "Adding: " << simpleCalc.add() << endl;
      cout << "Multiplying: " << simpleCalc.multiply() << endl;
      simpleCalc.setValue1( 7);
      cout << endl;

      Calculator<int,int> copyCalc( simpleCalc);
      cout << "Adding: " << simpleCalc.add() << endl;
      cout << "Multiplying: " << simpleCalc.multiply() << endl;
      cout << endl;

      Calculator<float,double> realCalc(1.41421, 2.718281828459045);
      cout << "Adding: " << realCalc.add() << endl;
      cout << "Multiplying: " << realCalc.multiply() << endl;
      cout << endl;
      return 0;
}

Don't flush where you don't need to.

Flushing is expensive, so don't use std::endl unless you need it.
Nearly always, the shorter '\n' suffices, which can be merged with any bordering string-literal.

Still, that advice seems to be for your teacher, not you ;-)
return 0; is implicit in main if control reaches the closing brace }.
The same is true for C since C99.

This also seems to be one for your teacher.
I see you corrected your formatting since posting on SO.

Still, 6 spaces is unusual. Either use the standard tab-stop (8), or if that's too much for your tastes, 4 is a common choice.

Automatic formatting-tools exist which will do this and much additional formatting for you, which gain importance with the size of the project, especially the number of contributors.
There is no reason to write the member-functions of the template out-of-line.

Even if you later divide your projects in multiple files, and thus have headers and implementation-files, templates are always (with really few exceptions) implemented in the header anyway, due to technical constraints.
There is absolutely no reason to explicitly implement the copy-constructor.
Let the compiler provide a default one.
Since C++11, one can also explicitly default it, preferably at the point of declaration: = default;.

Additional benefit: Doing so re-enables move-ctor and move-assignment.
The 2-argument-ctor should be templated to accept any assignable types and use perfect forwarding:

template <typename T3, typename T4, typename = decltype(T1(std::declval<T3&&>()),
    T2(std::declval<T4&&>()), void())>
Calculator(T3&& a, T4&& b) : number1(std::forward<T3>(a)), number2(std::forward<T4>(b)) {}

The setters should accept any assignable type and use perfect forwarding for optimal performance.

template <typename T3>
auto setValue1(T3&& x) -> decltype(number1 = std::forward<T3>(x), void())
{ number1 = std::forward<T3>(x); }
template <typename T3>
auto setValue2(T3&& x) -> decltype(number2 = std::forward<T3>(x), void());
{ number2 = std::forward<T3>(x); }

add and multiply should use a deduced return-type, and be const.
This transfers to avoiding explicit types in the implementations:

Use decltype(std::declval<T1&>()+std::declval<T2&>()) for C++11.

decltype(std::declval<T1&>()+std::declval<T2&>()) add() const;
decltype(std::declval<T1&>()*std::declval<T2&>()) multiply() const;

Or since C++14, just auto for letting the compiler deduce it.

auto add() const {
    auto answer = number1 + number2;
    return answer;
}

Or shorter:

auto add() const {
    return number1 + number2;
}

Putting it all together:

template <class T1, class T2>
class Calculator {
    T1 number1;
    T2 number2;
public:
    template <class T3, class T4, class = decltype(T1(std::declval<T3&&>()),
        T2(std::declval<T4&&>()), void())>
    Calculator(T3&& a, T4&& b) : number1(std::forward<T3>(a)), number2(std::forward<T4>(b)) {}
    // implicit: copy-ctor, move-ctor, copy-assignment, move-assignment, dtor

    template <class T3>
    auto setValue1(T3&& x) -> decltype(number1 = std::forward<T3>(x), void())
    { number1 = std::forward<T3>(x); }
    template <class T3>
    void setValue2(T3&& x) -> decltype(number2 = std::forward<T3>(x), void());
    { number2 = std::forward<T3>(x); }

    auto add() const { return number1 + number2; }
    auto multiply() const { return number1 * number2; }
};


find the maximum occur number in an integer Write a function named maxOccurDigit that returns the digit that
  occur the most. If there is no such digit,  it will return -1. For
  example maxOccurDigit(327277) would return 7 because 7 occurs three
  times in the number  and all other digits occur less than three times.

  Other examples:

  maxOccurDigit(33331) returns 3

  maxOccurDigit(3232, 6) returns -1

  maxOccurDigit(5) returns 5

  maxOccurDigit(-9895) returns 9

  The function signature is
  maxOccurDigit(int n)

I have done it this way:

public class checknumbers{
   public static void main(String[] args)  {
   System.out.println(maxOccurDigit(32332));
  }

 public static int maxOccurDigit(int a){
    String str2 = Integer.toString(a); // converts a into String
    char[] ch2 = str2.toCharArray(); //gets str2 into an array of char
    int[] t2 = new int[ch2.length];  //defines t2 for bringing ch2 into it
    for(int i=0;i<ch2.length;i++){ //  ASCII table
      t2[i]= (int) ch2[i]-48;   // ch2[i] is 48 units more than what we want
    }
    int count = 1, tempCount;
    int popular = t2[0];
    int temp = 0;
    for (int i = 0; i < (t2.length - 1); i++){
    temp = t2[i];
    tempCount = 0;
    for (int j = 1; j < t2.length; j++){
      if (temp == t2[j])
        tempCount++;
    }
    if (tempCount > count){
      popular = temp;
      count = tempCount;
    }
  }
    return popular;
  }
 }

How could I return -1 if there are two integers but methods have only one?

You are passed an integer parameter, and you have to return an integer.  I do not see it as worthwhile converting to a string for the counting.  Better to count the digits directly from the original integer.  MOD (%) 10 will give you the last integer, and DIV (/) 10 will shift the remainder along one position.

You may have to be careful with 0 and negative numbers with this method, otherwise you might get unexpected results.

As will as finding the most common digit you also have to find if there are any other digits with the same frequency.  As you look for a new higher maximum, look for other digits with the same count and keep a note if they exist.

This is my code:

public static int maxOccurDigit(int a) {

    // Zero is a special case.
    if (a == 0) {
        return 0;
    }

    // Numbers != 0.
    int[] digitCounts = new int[10];

    // Ensure a positive
    if (a < 0) {
        a *= -1;
    }

    while (a > 0) {
        digitCounts[a % 10] += 1;
        a /= 10;
    }

    // Find max count and occurrences.
    int maxCount = digitCounts[0];
    boolean sameCounts = false;
    int digit = 0;
    for (int i = 1; i < digitCounts.length; i++) {
        if (digitCounts[i] > maxCount) {
            maxCount = digitCounts[i];
            sameCounts = false;
            digit = i;
        } else if (digitCounts[i] == maxCount) {
            // Digits with equal counts.
            sameCounts = true;
        }
    }

    // Check for multiple max counts.
    if (sameCounts) {
        return -1;
    }

    // Valid single max digit
    return digit;

} // end maxOccurDigit()


Double click to edit and add new value to li $(document).on('dblclick', '#li', function () {
    oriVal = $(this).text();
    $(this).text("");
    input = $("<input type='text'>");
    input.appendTo('#li').focus();

});

$(document).on('focusout', 'input', function () {
    if (input.val() != "") {
        newInput = input.val();
        $(this).hide();
        $('#li').text(newInput);
    } else {
        $('#li').text(oriVal);
    }

});

So far, I still can't do OOP in jQuery.  Can you improve the above code?

I wasn't sure how to answer this, so I went with what your code does...

Scope your variables.
Change your code to target all li instead of one element with the id li.
Use this when events are triggered to make sure you have the correct element.
Remove the added elements instead of hiding them.
Remember to call on using the smallest possible parent container. Possibly a <ul> or <ol>?
Use short circuiting where appropriate.
Use jQuery.parent() to get the appended elements parent.

Putting all those change together:

var oriVal;
$("#parentUL").on('dblclick', 'li', function () {
    oriVal = $(this).text();
    $(this).text("");
    $("<input type='text'>").appendTo(this).focus();
});
$("#parentUL").on('focusout', 'li > input', function () {
    var $this = $(this);
    $this.parent().text($this.val() || oriVal); // Use current or original val.
    $this.remove();                      // Don't just hide, remove the element.
});

Here's the JSFiddle.


Given two sorted arrays, add the elements of the second array into the first In Cracking the Coding Interview by Gayle Laakmann McDowell, there's a question that asks you to write code for the following:

  Given two sorted arrays, A and B. Write a method merging the elements
  of B into A in sorted order. Assume A has a large enough buffer at the
  end to hold all of B's elements.

Though the problem isn't very hard, I've been striving to increase the readability of my code in general. As such, I wrote two versions of my solution, and am looking for input on which version is most readable.

Version 1:

// Given two sorted arrays, adds the elements of the second array into
// the first, while maintaining ordering property.
// XXX: Assumes first array has enough buffer space to hold all elements
public static <Type extends Comparable<Type>> Type[]
mergeIntoFirstArray(Type[] first_array, Type[] second_array) {

    int first_idx         = numElementsInArray(first_array) - 1;
    int second_idx        = second_array.length - 1;
    int merge_idx        = numElementsInArray(first_array) + second_array.length - 1;    //

    // merge largest values first, until either array is exhausted
    while (first_idx >= 0 && second_idx >= 0) {
        Type first_value    =  first_array[first_idx];
        Type second_value    =  second_array[second_idx];

        // add the largest value
        if (first_value.compareTo(second_value) > 0){
            first_array[merge_idx] = first_value ;
            first_idx--;
        } else {
            first_array[merge_idx] = second_value ;
            second_idx--;
        }
        merge_idx-- ;
    }

    // if second_array still has values, merge them in
    while (second_idx >=0) {
        first_array[merge_idx] = second_array[second_idx] ;
        merge_idx-- ;
        second_idx--;
    }

    return first_array ;
}

Version 2:

public static <Type extends Comparable<Type>> Type[]
mergeIntoFirstArrayV2(Type[] first_array, Type[] second_array) {
    int f    = numElementsInArray(first_array)-1;
    int s    = second_array.length - 1;

    // merge in largest values first
    for (int merge_idx = f + s + 1 ; merge_idx >= 0 ; merge_idx--) {
        Type fvalue    =  f >= 0 ? first_array[f]  : null;
        Type svalue    =  s >= 0 ? second_array[s] : null;

        // first value exists, and is greater than second value
        if (fvalue != null &&
            (svalue == null || fvalue.compareTo(svalue) > 0)) {
            first_array[merge_idx] = svalue ;
            s_idx--;
        }
        else {
            first_array[merge_idx] = svalue ;
            s_idx--;
        }
    }

    return first_array ;
}

Any meaningful input on improving the readability of either method would also be appreciated. For one, I think my variable names might be unclear or too long in some cases. For version 2, I suspect that though the code is more concise, it is harder to understand at a glance.

Both are equivalently readable, I'll nitpick on some details though :)

Both solutions will fail with null elements.

The generic type would be better named T instead of Type, as Type has another definition already.

numElementsInArray() as a function looks a little odd because it is undefined, instead consider passing in the information you need as part of the method definition.

When modifying in place you do not want to return the array because presumably you are trying to skimp on memory allocations and are just going to look for the result in the first array that was passed in.


CakePHP view with interleaved PHP and HTML It's CakePHP, though I'm more interested in what people think about the layout of the indentation, the mark-up, position of logic etc.

Any feedback that relates to the subject of writing CakePHP views / front end code should take precedence over CakePHP feedback or logic feedback, though the latters are welcome too

<?php foreach ($list as $l): ?>

    <?php $english=$l['translation']['english']; ?>

    <?php $translate_to=$l['translation']['translate_to']; ?>

    <?php $content=$l['Content']; ?>

    <?php if($l['flag_count']>0): ?>

                        <?=$l['flag_count'] ?>

    <?php else: ?>

                        <?=$l['flag_count'] ?>

    <?php endif; ?>

                        <?=$this->Html->link("Edit",$l['edit_route']) ?>

                            <?=$english[$content['content_type']."_name"] ?>

    <?php if ($english_id!=$current_language_id): ?>

                            <?=$translate_to[$content['content_type']."_name"] ?>

    <?php endif; ?>

    <?php if ('range'==$content['content_type']): ?>

        <?php $slider_class=(1==$content['slider']) ? "in_slider" : ""; ?>

        <?php $slider_text=(1==$content['slider']) ? "on home" : ""; ?>

                        <div class='js_slider_toggle' data-content-id='<?=$content['id'] ?>'>

                                display on home?

                            <span class='js_icon slider_toggle <?=$slider_class ?>'>

    <?php endif; ?>

<?php endforeach; ?>

Why not go with a template like Twig or Blade which would make the syntax a lot cleaner, while retaining most of all the controls. Also, remember that the view is for showing data, try to maintain as much logic OUT of the view as possible.

Here's an untested version of your code with Blade:

    @foreach($list as $l)

        @if($l['flag_count'] > 0)
            {{ $l['flag_count'] }}
        @else
            {{ $l['flag_count'] }}
        @endif

        {{ $this->Html->link("Edit", $l['edit_route']) }}

            {{ $l['translation']['english'][ $l['Content']['content_type'] . "_name" ] }}

        @if($l['translation']['english'] != $current_language_id)

                {{ $l['translation']['translate_to'][ $l['Content']['content_type'] . "_name" ] }}

        @endif

        @if('range' === $l['Content']['content_type'])

                display on home?

        @endif

    @endforeach


Get information about the USB device from Win32_DiskDrive and Win32_LogicalDisk I want to know the information about a USB device from Win32_DiskDrive and Win32_LogicalDisk.

I use the following code:

private void Add(string letter) // letter is a logical name of the USB device, e.g. "G:\\"
{
  foreach (var drive in new ManagementObjectSearcher("SELECT DeviceID, SerialNumber, Partitions FROM Win32_DiskDrive WHERE InterfaceType='USB'").Get())
  {
    var partition = new ManagementObjectSearcher(string.Format("ASSOCIATORS OF {{Win32_DiskDrive.DeviceID='{0}'}} WHERE AssocClass = Win32_DiskDriveToDiskPartition", drive["DeviceID"])).First();

    if (partition != null)
    {
      var logical = new ManagementObjectSearcher(string.Format("ASSOCIATORS OF {{Win32_DiskPartition.DeviceID='{0}'}} WHERE AssocClass = Win32_LogicalDiskToPartition", partition["DeviceID"])).First();

      if (logical != null)
      {
        var volume = new ManagementObjectSearcher(string.Format("SELECT Name, FreeSpace FROM Win32_LogicalDisk WHERE Name='{0}'", logical["Name"])).First();

        string name = (string)volume["Name"];
        string serialNumber = (string)drive["SerialNumber"];
        ulong freeSpace = (ulong)volume["FreeSpace"];
        break;
      }
    }
  }
}

  public static class WmiExtensions
  {
    // Fetch the first item from the search result collection.
        public static ManagementObject First(this ManagementObjectSearcher searcher)
        {
          ManagementObject result = null;
          foreach (ManagementObject item in searcher.Get())
          {
            result = item;
            break;
          }
          return result;
        }
      }
    }

Is there a way to make it shorter and more elegant?

1: Add() doesn't appear to access any class members, so it can be declared static. But then again, it looks like it wants to return the name, serial number, and free space. Make it do so - create a quick container type

public sealed class DriveInfo
{
    public DriveInfo(string name, string serialNumber, ulong freeSpace)
    {
        this.Name = name;
        this.SerialNumber = serialNumber;
        this.FreeSpace = freeSpace;
    }

    public string Name { get; }

    public string SerialNumber { get; }

    public ulong FreeSpace { get; }
}

usage becomes:

return new DriveInfo(
    (string)volume["Name"],
    (string)drive["SerialNumber"],
    (ulong)volume["FreeSpace"]);

2: The parameter letter is completely unused in the method and the comment is a lie. Remove the parameter and comment or use the parameter appropriately.

3: Invert your if blocks, to prevent so much indention as well as keeping the tests looking cleaner (equality vs. non-equality):

if (partition == null) continue;
...
if (logical == null) continue;

4: Both ManagementObject and ManagementObjectSearcher descend from Component, which implements IDisposable. Their lifetime should therefore be wrapped in a using statement:

using (var searcher = new ManagementObjectSearcher($"ASSOCIATORS OF {{Win32_DiskDrive.DeviceID='{drive["DeviceID"]}'}} WHERE AssocClass = Win32_DiskDriveToDiskPartition"))
using (var partition = searcher.First())
{
}

5: Your extension method's body can be replaced by a quick one-liner:

return searcher.Get().Cast<ManagementObject>().FirstOrDefault();


isRotation and isSubstring method I have written an isRotation function that calls on isSubstring only once(I am allowed to call it only once). I have implemented my own version of isSubstring as well. Any advice is much welcome!

//Checks if one string is a rotation of the other
#include <stdio.h>
#include <string.h>
#include <stdbool.h>

bool isRotation(char *, char *);
bool isSubstring(char *, char *);

int main(int argc, char **argv)
{
    if(argc == 3)
    {
        printf("%s %s a rotation of %s.\n", argv[2], isRotation(argv[1], argv[2])
            ? "is" : "is not", argv[1]);
    }
    else
    {
        fprintf(stderr, "Incorrect number of inputs\n");
        return -1;
    }

    return 0;
}

bool isRotation(char* s1, char* s2)
{
    size_t len1 = strlen(s1);
    size_t len2 = strlen(s2);

    if(len1 != len2 || len1 == 0 || len2 ==0)
        return 0;

    char s3[len2*2];

    bool secondCopy = false;
    int j = 0;

    /*copying 2*s2 into s3. If s2 is a rotation of s1,
      then s1 has to be found somewhere inside s3.*/
    for(size_t i = 0; i < (2*len2); i++)
    {
        s3[i] = s2[j];
        if(s2[j+1] == '\0' && secondCopy == false)
        {
            secondCopy = true;
            j = 0;
            continue;
        }
        j++;
    }

    s3[len2*2] = '\0';

    //pass the longer string to the first param
    return isSubstring(s3, s1);
}

bool isSubstring(char* s1, char* s2)
{
    //is s2 a substring of s1?
    size_t len2 = strlen(s2);
    if(len2 == 0)
        return true;

    int j = 0;
    int i = 0;
    while(s1[i] != '\0')
    {
        if(s2[j] == s1[i])
            j++;

        i++;
    }

    return j == len2;
}

Better string copy
You are over-complicating your string duplication. There is an easier way to do it using memcpy. First, simply copy the string:
memcpy(s3, s2, len2);

Then, simply copy the string again (this time, with an offset):
memcpy(s3 + len2, s2, len2);

Notice how the second copy doesn't have - in the last argument. With it like this, you do not have to manually copy the null character over afterwards as it will use the null character from s2.
Note that you can not do this as easily with strcpy as this function will copy the entire string, including the null character.

Not true substring
Your isSubstring function doesn't actually find the substring of a string very well. Take these two strings as an example:

abec abc

Your function would return true to these if these were passed in in this order. A simple fix to this would be to reset j to 0 every time a character is encountered that is not the right one.
while(s1[i] != '\0')
{
    if(s2[j] == s1[i]) {
        j++;
    } else {
        j = 0;
    }

    i++;
}

Misc

Returning bool

return 0;

Why are you returning a number from a bool-returning method?

Clear up the for loop

int j = 0;
...
for(size_t i = 0; i < (2*len2); i++)
{
    ...
    j++;
}

This is somewhat confusing. You should move the j declaration right above the for loop, and move the incrementing into the signature.

Refactor
The string duplication logic should be moved into an external function as to spread out the logic and make sure that every function is only doing what it needs to do and nothing else.

Braces
Always use them, even around single-line statements. They make your code more maintainable and less error-prone.

While to for loop
This:

int j = 0;
int i = 0;
while(s1[i] != '\0')
{
    if(s2[j] == s1[i])
        j++;

    i++;
}

Can be written more cleanly as a for loop:
for(int i = 0, j = 0; s1[i] != '\0'; i++) {
    if(s2[j] == s1[i])
        j++;
}


Removing code smell from Room Booking code I am new with coding and I recently read an article about code refactoring, so I have made a console application for booking rooms on a ship. I need help with code refactoring. As far as I understand the refactoring, I think in my project there are only 2 parts where I need refactoring, which are as follows.

One is a block of for statements:

    ship1 = new Ship("Olympic Countess");

    ArrayList groupA = new ArrayList();
    for (int i = 0; i < 10; i++)
    {
        groupA.Add(new room(5000, "A" + (i + 1)));
    }

    ArrayList groupB = new ArrayList();
    for (int i = 0; i < 10; i++)
    {
        groupB.Add(new room(4000, "B" + (i + 1)));
    }

    ArrayList groupC = new ArrayList();
    for (int i = 0; i < 30; i++)
    {
        groupC.Add(new room(3500, "C" + (i + 1)));
    }

    ArrayList groupD = new ArrayList();
    for (int i = 0; i < 36; i++)
    {
        groupD.Add(new room(3400, "D" + (i + 1)));
    }

    ArrayList groupE = new ArrayList();
    for (int i = 0; i < 40; i++)
    {
        groupE.Add(new room(3300, "E" + (i + 1)));
    }

    ArrayList groupF = new ArrayList();
    for (int i = 0; i < 30; i++)
    {
        groupF.Add(new room(3400, "F" + (i + 1)));
    }

    ArrayList groupG = new ArrayList();
    for (int i = 0; i < 36; i++)
    {
        groupG.Add(new room(3300, "G" + (i + 1)));
    }

    ArrayList groupH = new ArrayList();
    for (int i = 0; i < 40; i++)
    {
        groupH.Add(new room(3200, "H" + (i + 1)));
    }

    ship1.addDeck("Balcony Suite", groupA);
    ship1.addDeck("Suite", groupB);
    ship1.addDeck("Deck 3 - Outside Twin", groupC);
    ship1.addDeck("Deck 2 - Outside Twin", groupD);
    ship1.addDeck("Deck 1 - Outside Twin", groupE);
    ship1.addDeck("Deck 3 - Inside Twin", groupF);
    ship1.addDeck("Deck 2 - Inside Twin", groupG);
    ship1.addDeck("Deck 1 - Inside Twin", groupH);
}

and the other one is the if else statement as follows:

public Reservation bookPassage(String cabinclass, Customer booker, int number)
  {
    ArrayList cabins;
    if (cabinclass.Equals("a", StringComparison.OrdinalIgnoreCase))
        cabins = ship1.getDeck("Balcony Suite");
    else if (cabinclass.Equals("b", StringComparison.OrdinalIgnoreCase))
        cabins = ship1.getDeck("Suite");
    else if (cabinclass.Equals("c", StringComparison.OrdinalIgnoreCase))
        cabins = ship1.getDeck("Deck 3 - Outside Twin");
    else if (cabinclass.Equals("d", StringComparison.OrdinalIgnoreCase))
        cabins = ship1.getDeck("Deck 2 - Outside Twin");
    else if (cabinclass.Equals("e", StringComparison.OrdinalIgnoreCase))
        cabins = ship1.getDeck("Deck 1 - Outside Twin");
    else if (cabinclass.Equals("f", StringComparison.OrdinalIgnoreCase))
        cabins = ship1.getDeck("Deck 3 - Inside Twin");
    else if (cabinclass.Equals("g", StringComparison.OrdinalIgnoreCase))
        cabins = ship1.getDeck("Deck 2 - Inside Twin");
    else
        cabins = ship1.getDeck("Deck 1 - Inside Twin");

What I don't understand is that my parameters are changing in both logic, so how can I make a separate method for this logic when my cabin class is changing every time?

None of this information should be written into your program's source code.  Rather, it should be stored in a configuration file, perhaps in XML, JSON, or YAML format  — or maybe stored in a database instead.  Not only would your code be less repetitive, it would also be more flexible, in that you wouldn't have to rebuild the application just to change trivial things such as product offerings and prices.


React code to build shopping cart I have built a basic shopping cart simulator. All it does is add and remove items between two different basket (i.e. aisle vs basket).

While working on the problem I started to think about the approach and would love to hear what people think.

Avoid initializing the state empty state in the constructor. However, I had to initialize shoppingCart: [] in componentDidMount hook to avoid type error. What is the better and cleaner approach?
Is it good practice to keep the logic simple in handleXXXClicks, separation of concern one for handling items and one for shoppingCart?
I feel aisleList and cartList bloated and I could have written itemComponent which is avoidable duplicate code.
Is there any better way to handle the item in setState? I couldn't think of using prevState in my scenario.

Use setState without initializing state:

componentDidMount() {
    // assuming data comes from API fetch
    let items = ["banana", "eggs", "bread", "apple"];
    this.setState({
      items,
      shoppingCart: []  // had to set this empty to avoid typeerror
    });
  }

Handle handleClick and setState:

handleCartClick = event => {
    let newList = this.state.shoppingCart.filter(item => {
      return item !== event.target.value;
    });

    this.setState({
      items: [...this.state.items, event.target.value],
      shoppingCart: [...newList]
    });
  };

Gist link

Demo link

Some Tips

Avoid using constructor.
Avoid using .bind(this)
Destructuring will help you to make cleaner code.

Questions

  Avoid initializing the state empty state in the constructor. However, I had to initialize shoppingCart: [] in componentDidMount hook to avoid type error. What is the better and cleaner approach?

You can have 2 aproaches to this

There is a way to avoid this, what you do is destruct the state in the variables you want and set then a default value of an array.

state = {}

render() {
    const {
        shoppingCart = []
    } = this.state
    ...
}

This way you don't need to check if shoppingCart is an array, and you only need to declare this.state as an object ( it can be just an empty object ).

You can also do

render() {
        const {
        shoppingCart = []
    } = this.state || {}
    ...
}

And now you don't even need state = {}
Because you are using arrow functions, you don't need .bind(this) and also you can avoid de constructor and just use

state = {
    ...
}

This way you can declare shoppingCart and items and don't need to declare it in componentDidMount (wich is much better to understand) and now don't need to destruct the state.

state = {
    shoppingCart = [],
    items = []
}

This way is good if the data you will get is always an array (or the type you specify) and another good reason to use this is that when other programmer look at the code, they will immediately know what is stored in that component state, but when you have too many things in the state, it can get confused.

This aproaches will solve some problems like using ternary to check if you have an state and then return the mapping or if you don't have, return an empty array.

render() {

    const {
        items = [],
        shoppingCart = []
    } = this.state

    let aisleList = items.map((item, index) => {
        return (

                {item}
                <input
                    type="checkbox"
                    id={index}
                    value={item}
                    onClick={this.handleAileClick}
                />

        );
    });

    let cartList = shoppingCart.map((item, index) => {
        return (

                {item}
                <input
                    type="checkbox"
                    autoComplete="off"
                    value={item}
                    onClick={this.handleCartClick}
                />

        );
    });
    ...
}

Looking at this, is much clean and easy to understand.

  Is it good practice to keep the logic simple in handleXXXClicks, separation of concern one for handling items and one for shoppingCart?

You can do the way you want, but you need to see what you will do in the future, if both will always do the same, you can create one function for both and just send a different parameter, but if they will do some extra stuff, it's good to have then separated.

*(I'm not english native speaker and maybe I didn't understand exaclty what is this part about).

  I feel aisleList and cartList bloated and I could have written itemComponent which is avoidable duplicate code.

Yes, they look so similar that you should create a component for both and pass the diference in props.

  Is there any better way to handle the item in setState? I couldn't think of using prevState in my scenario.

Here is simple setState using a function with prevState. Note that the way you are doing it, you need to have event.persist(), for more info look here

handleCartClick = event => {
    event.persist();

    this.setState(prevState => {
        return {
            items: [...prevState.items, event.target.value],
            shoppingCart: prevState.shoppingCart.filter(item => {
                return item !== event.target.value;
            })
        };
    });
};

Overall

Your code looks ok, there is only one strange thing. You are using checkboxes and when you click, some get marked and other don't. I don't know if that was a test or you also wanted to solve the checkbox problem. Let me know and I can see what I can do.

I recommend you to start using some new things like I mentioned in the tips part, that help you alot and make your code cleaner.

Also have a look at hooks.
For me, using hooks makes alot of things easier, but also, the code looks cleaner.

I made a demo for you to test and see the new code. I changed some things.

I changed handleCartClick but left handleAileClick the way it was before, so you can try useing prevState yourself.
And also left the part of itemComponent for you.

The best way to learn, is doing it!

Hope I helped you!


Wrapping around window.console I'm working on an old application written in JavaScript. It's absolutely littered with console.* messages some of which are useful for development purposes.

At the very least I'd like to switch these off when a production flag is set. I would also like to stub off any unsupported console methods (trace on IE for example). Note: Assume that we will always have ES5 support.

Concerns:

I call into the initializer as soon as the constructor is instantiated. I like the idea of having a separate initializer method because it can be swapped out. I don't however like the idea that subsequent calls can be made. Any thoughts on this? I've seen it used a lot in Backbone's source.
As you can see I've tried to keep the production flag separate from the general 'enabled' flag. When in production I noop all of the methods instead of wrapping them (or nooping them if unsupported). Perhaps there's a better way of catching all calls to console (like a jasmine style spy) in production?
My hope with wrapping each supported method was to have a way of injecting pre-conditions/formatting before it's fired off. I also use this as an opportunity to check flags like enable. Is this over-contrived?

var Logger = function () {
    this._methods = [
        'assert', 'clear', 'count', 'debug', 'dir', 'dirxml',
        'error', 'exception', 'group', 'groupCollapsed', 'groupEnd',
        'info', 'log', 'markTimeline', 'profile', 'profiles',
        'profileEnd', 'show', 'table', 'time', 'timeEnd',
        'timeline', 'timelineEnd', 'timeStamp', 'trace', 'warn'
    ];

    this.initialize.apply(this, arguments);
};

var console = window.console, p = Logger.prototype;

p.initialize = function (options) {
    var noop = function () {};

    this.options = options || {};

    this._enabled = !this.options.production;

    this._methods.forEach(function (method) {
        if (this.options.production) {
            this[method] = noop;
        } else {
            this[method] = console[method] ? this._wrap(method) : noop;
        }
    }.bind(this));
};

p._wrap = function (method) {
    return function () {
        if (this._enabled) {
            return console[method].apply(console, arguments);
        }

        return false;
    };
};

p.enable = function () {
    if (!this.options.production) {
        this._enabled = true;
    }
};

p.disable = function () {
    this._enabled = false;
};

Interesting question,

I spent a bit of time coding different approaches before writing this. Initially I just was going to point to my SO answer but then I saw you have some more requirements that are not met.

Concern #1

I dislike libraries where I have to initialize stuff outside of the constructor, the constructor should initialize an object so that it is usable! Why give me a half baked object that I cannot use? The idea of being able to swap out a separate initializer is typical YAGNI.

Concern #2

In my mind, the caller should decide whether to enable or disable (possibly based on a production flag or not), for your code to take care of both concerns independantly seems overkill.

Concern #3

It seems over-contrived to me ;)

I did like though the idea of fool-proofing all console methods for an ancient legacy code base, and kept that with 2 simple methods to turn of or on all output in my approach:

function logController(){

  var methods = [
    'assert', 'clear', 'count', 'debug', 'dir', 'dirxml',
    'error', 'exception', 'group', 'groupCollapsed', 'groupEnd',
    'info', 'log', 'markTimeline', 'profile', 'profiles',
    'profileEnd', 'show', 'table', 'time', 'timeEnd',
    'timeline', 'timelineEnd', 'timeStamp', 'trace', 'warn'
  ];
  //The one acceptable time we can pollute the global namespace ;]
  console = console || {};

  function stop(){
    var _method;
    methods.forEach( function (method){
      _method = '_' + method;
      console[_method] = console[_method] || console[method];
      console[ method] = function noLog(){};
    });
  }

  function start(){
    var _method;
    methods.forEach( function (method){
      _method = '_' + method;
      console[method] = console[_method] || console[method] || function noLog(){};
    });
  }
  start();

  return {
    start: start,
    stop: stop
  };
}

flow = logController();
console.log( 'Normal logging, next is 123' );
console.log(  123 );
console.log( 'Start logging (which is default state), next is 123' );
flow.start();
console.log(  123 );
console.log( 'Stop logging (which is default state), next is "start logging again"' );
flow.stop();
console.log(  123 );
flow.start();
console.log( 'Started logging (which is default state), next is 123' );
console.log(  123 );

I will keep the exercise to make this work with AMD/UMD in your capable hands ;)


Grep to search for array of location targets Please review my short 65 lines of code:

Is there a way to avoid eval (security)?
Is there a better way or place to use shopt -s nullglob?
Does shopt -u nullglob always need to be unset through each iteration?
Is there a smarter way to define a list of locations?

#!/bin/bash
if [ $# -eq 0 ]
then
    echo specify at least a regular expression pattern
    exit 1
fi
# make an array. Use 'single quotes'
SEARCH_LOCATIONS=(
'$BIN_HOME/*'
'$PBIN_HOME/*'
'$PBIN_HOME/lib/*'
'$SH_HOME/* '
'$SH_HOME/*/*'
'$PROD_HOME/psp/*.psp'
'$PROD_HOME/sql/*.sql'
'$PROD_HOME/sql/*/*.sql'
'$PROD_HOME/rpt/[A-Z]*.rpt'
'$PROD_HOME/script/*'
'$PROD_HOME/fmb/[A-Z]*.pll'
'$PROD_HOME/xml/[A-Z]*.pld'
'$PROD_HOME/fmb/[A-Z]*.mmb'
'$PROD_HOME/xml/[A-Z]*.xml'
'$PROD_HOME/cgi/*'
'$ELETTUS_HOME/*'
'$ELETTUS_HOME/*/*'
'$PROD_HOME/guicommon/*'
)
for LOCATION in ${SEARCH_LOCATIONS[@]}
do
    # ---- evaluate array contents
    echo --------------------------------------------------------------------------------
    echo "$LOCATION"
    echo
    eval declare location=$LOCATION
    if [[ $location != /u2/* ]]
    then
        echo $LOCATION is not a part of Lettus system >&2
        echo
        continue
    fi
    shopt -s nullglob
    grep -E -I -d skip "$@" $location
    shopt -u nullglob
done

Your questions:

Yes. Use double quotes and then drop the eval (this will also cause the script to show /path/to/prod/home instead of $PROD_HOME)
Yes, just do it once at the start of the script.
No.
Depends on your requirements.

Other issues:

Using multiple regex can be specified but won't work
Using paths with spaces doesn't work

Keeping most of your current functionality, I probably do something like this:

#!/bin/bash
shopt -s nullglob

# Allow using ** to recursively expand globs
shopt -s globstar

# Don't do any field splitting
IFS=''

if (( ! $# ))
then
    echo "specify at least one regex"
    exit 1
fi

locations=(
  "$BIN_HOME/*"
  "$PBIN_HOME/**"
  "$SH_HOME/**"
  "$PROD_HOME/psp/*.psp"
  "$PROD_HOME/sql/*.sql"
  "$PROD_HOME/sql/*/*.sql"
  "$PROD_HOME/rpt/[A-Z]*.rpt"
  "$PROD_HOME/script/*"
  "$PROD_HOME/fmb/[A-Z]*.pll"
  "$PROD_HOME/xml/[A-Z]*.pld"
  "$PROD_HOME/fmb/[A-Z]*.mmb"
  "$PROD_HOME/xml/[A-Z]*.xml"
  "$PROD_HOME/cgi/*"
  "$ELETTUS_HOME/**"
  "$PROD_HOME/guicommon/*"
)

# Quote all variables unless you specifically can't
for location in "${locations[@]}"
do
    echo "--------------------------------------------------------------------------------"
    # Printf generally works better for arbitrary data, though this is unlikely to be an issue in your case
    printf "%s\n" "$location"
    echo
    if [[ "$location" != /u2/* ]]
    then
        printf "%s is not a part of Lettus system\n" "$location" >&2
        echo
        continue
    fi

    # grep can accept multiple patterns separated by line feeds
    # Add a dummy file so non-matching globs won't cause grep to read from stdin
    # Here we want $location to glob expand, so no quotes (IFS was set earlier to prevent field splitting)
    grep -E -I -d skip "$(printf "%s\n" "$@")" /dev/null $location
done